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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2022, 15:40 UTC
Message-ID: <xmqq5ykignwb.fsf@gitster.g>
In-Reply-To: <220630.86v8siclh5.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 33 quoted lines

> 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?

[Footnote]

*1*

    Tests on Windows: $(pwd) must return Windows-style paths

    Many tests pass $(pwd) in some form to git and later test that the output
    of git contains the correct value of $(pwd). For example, the test of
    'git remote show' sets up a remote that contains $(pwd) and then the
    expected result must contain $(pwd).

    Again, MSYS-bash's path mangling kicks in: Plain $(pwd) uses the MSYS style
    absolute path /c/path/to/git. The test case would write this name into
    the 'expect' file. But when git is invoked, MSYS-bash converts this name to
    the Windows style path c:/path/to/git, and git would produce this form in
    the result; the test would fail.

    We fix this by passing -W to bash's pwd that produces the Windows-style
    path.

    There are a two cases that need an accompanying change:

    - In t1504 the value of $(pwd) becomes part of a path list. In this case,
      the lone 'c' in something like /foo:c:/path/to/git:/bar inhibits
      MSYS-bashes path mangling; IOW in this case we want the /c/path/to/git
      form to allow path mangling. We use $PWD instead of $(pwd), which always
      has the latter form.

    - In t6200, $(pwd) - the Windows style path - must be used to construct the
      expected result because that is the path form that git sees. (The change
      in the test itself is just for consistency: 'git fetch' always sees the
      Windows-style path, with or without the change.)

    Signed-off-by: Johannes Sixt <j6t@kdbg.org>