Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Han Xin <hanxin.hx@bytedance.com>,
	chiyutianyi@gmail.com, derrickstolee@github.com,
	git@vger.kernel.org, haiyangtand@gmail.com,
	jonathantanmy@google.com, me@ttaylorr.com, ps@pks.im
Subject: Re: [PATCH v3 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()
Date: Thu, 30 Jun 2022 20:47:30 +0200	[thread overview]
Message-ID: <220630.86edz6c75c.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq5ykignwb.fsf@gitster.g>

On Thu, Jun 30 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Jun 30 2022, Johannes Schindelin wrote:
>>
>>> Hi Junio,
>>>
>>> On Tue, 28 Jun 2022, Junio C Hamano wrote:
>>>
>>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>>>
>>>> >> +test_expect_success 'setup: prepare a repository with commit-graph contains the commit' '
>>>> >> +	git init with-commit-graph &&
>>>> >> +	echo "$(pwd)/with-commit/.git/objects" \
>>>> >> +		>with-commit-graph/.git/objects/info/alternates &&
>>>> >
>>>> > nit: you can use $PWD instead of $(pwd).
>>>>
>>>> We can, and it would not make any difference on non-Windows.
>>>>
>>>> But which one should we use to cater to Windows?  $(pwd) is a full
>>>> path in Windows notation "C:\Program Files\Git\..." while $PWD is
>>>> MSYS style "/C/Program Files/Git/..." or something like that, IIRC?
>>>
>>> Indeed, and since the `alternates` file is supposed to be read by
>>> `git.exe`, a non-MSYS program, the original was good, and the nit
>>> suggested the incorrect form.
>>
>> I looked at t5615-alternate-env.sh which does the equivalent of:
>>
>> 	GIT_ALTERNATE_OBJECT_DIRECTORIES="$PWD/one.git/objects:$PWD/two.git/objects" \
>>         	git cat-file [...]
>>
>> We run that test on all our platforms, does the $PWD form work in the
>> environment variable, but not when we write it to the "alternates" file?
>> Or is there some other subtlety there that I'm missing?
>
> I am also curious to see a clear and concise explanation so that we
> do not have to repeat this discussion later.  We have
>
>  - When a test checks for an absolute path that a git command generated,
>    construct the expected value using $(pwd) rather than $PWD,
>    $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
>    Windows, where the shell (MSYS bash) mangles absolute path names.
>    For details, see the commit message of 4114156ae9.
>
> in t/README, but even with the log mesasge of 4114156a (Tests on
> Windows: $(pwd) must return Windows-style paths, 2009-03-13) [*1*],
> I have no idea what makes the thing you found in t5615 work and your
> suggestion to use $PWD in the new one not work.
>
> GIT_ALTERNATE_OBJECT_DIRECTORIES is a PATH_SEP (not necessarily a
> colon) separated list, and I think the way t5615 uses it is broken
> on Windows where PATH_SEP is defined as semicolon without the $PWD
> vs $(pwd) issue.  Is the test checking the right thing?

Whatever th explanation is CI passed with a $(pwd) -> $PWD repacement in
the test being introduced here:
https://github.com/avar/git/runs/7136686130?check_suite_focus=true

Diff here:
https://github.com/avar/git/commit/606d0060a57b7021396919044c7696489b7835cd

So either $PWD is fine there, or our Windows CI doesn't reflect this
particular caveat on some Windows systems, or the test is erroneously
passing with an invalid value. Knowing which of those it is would be
very useful...