[PATCH] t7900: use pwd -P in macOS maintenance test

[PATCH] t7900: use pwd -P in macOS maintenance test

[Mark Mentovai]

$pfx is the basis for the expectation that launchd plist paths formed by
`git maintenance start` will be compared against. These paths are formed
in `git maintenance` by builtin/gc.c launchctl_service_filename(), which
calls path.c interpolate_path() with real_home = 1, causing abspath.c
strbuf_realpath() to resolve a canonical absolute path. Since $pfx is
not determined according to the same realpath semantics, when t7900 is
run from a working directory that contains a symbolic link in its path,
the realpath operation will produce a different path than $pfx contains,
although both paths logically reference the same directory. The test
fails in this case.

Base $pfx on the physical working directory (pwd -P), with all symbolic
links fully resolved, so that the path that the test expects matches
what `git maintenance` generates, even when running from a working
directory whose path contains a symbolic link.

Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 t/t7900-maintenance.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 8cf89e285f49..677e92f1490e 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -882,7 +882,7 @@ test_expect_success 'stop preserves surrounding schedule' '
 
 test_expect_success 'start and stop macOS maintenance' '
 	# ensure $HOME can be compared against hook arguments on all platforms
-	pfx=$(cd "$HOME" && pwd) &&
+	pfx=$(cd "$HOME" && pwd -P) &&
 
 	write_script print-args <<-\EOF &&
 	echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args
-- 
2.49.0

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Eric Sunshine]

On Fri, May 23, 2025 at 3:37 PM Mark Mentovai <mark@chromium.org> wrote:
> $pfx is the basis for the expectation that launchd plist paths formed by
> `git maintenance start` will be compared against. These paths are formed
> in `git maintenance` by builtin/gc.c launchctl_service_filename(), which
> calls path.c interpolate_path() with real_home = 1, causing abspath.c
> strbuf_realpath() to resolve a canonical absolute path. Since $pfx is
> not determined according to the same realpath semantics, when t7900 is
> run from a working directory that contains a symbolic link in its path,
> the realpath operation will produce a different path than $pfx contains,
> although both paths logically reference the same directory. The test
> fails in this case.
>
> Base $pfx on the physical working directory (pwd -P), with all symbolic
> links fully resolved, so that the path that the test expects matches
> what `git maintenance` generates, even when running from a working
> directory whose path contains a symbolic link.
>
> Signed-off-by: Mark Mentovai <mark@chromium.org>
> ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -882,7 +882,7 @@ test_expect_success 'stop preserves surrounding schedule' '
>  test_expect_success 'start and stop macOS maintenance' '
>         # ensure $HOME can be compared against hook arguments on all platforms
> -       pfx=$(cd "$HOME" && pwd) &&
> +       pfx=$(cd "$HOME" && pwd -P) &&

Okay, this seems like the minimum fix[*], and -P is POSIX.

However, have you tested this on Windows? I ask because, despite the
test's name, this and most of the tests in this script, are actually
run on all platforms, and because `pwd` is overridden by a shell
function for MinGW on Windows:

    # t/test-lib.sh
    ...
    # git sees Windows-style pwd
    pwd () {
        builtin pwd -W
    }

My quick testing suggests that this patch's change might be problematic:

    # on Windows
    $ pwd
    /home/me
    $ pwd -W
    C:/msys64/home/me
    $ pwd -P
    /home/me
    $ pwd -W -P
    /home/me

FOOTNOTES

[*]: In the long run, a better fix would probably be for the tests to
sanitize the output of the Git command, replacing (via `sed`) the
actual emitted path with some placeholder, such as "%HOME%" or
something, and then have the tests look for (`grep` or whatnot)
needles using that literal placeholder rather than trying to perfectly
match the path emitted by Git. This approach makes sense since these
tests are about overall functionality of git-maintenance, not about
the specific path in which the person happens to be running the tests.

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> -       pfx=$(cd "$HOME" && pwd) &&
>> +       pfx=$(cd "$HOME" && pwd -P) &&
>
> Okay, this seems like the minimum fix[*], and -P is POSIX.
>
> However, have you tested this on Windows? I ask because, despite the
> test's name, this and most of the tests in this script, are actually
> run on all platforms, and because `pwd` is overridden by a shell
> function for MinGW on Windows:
>
>     # t/test-lib.sh
>     ...
>     # git sees Windows-style pwd
>     pwd () {
>         builtin pwd -W
>     }
>
> My quick testing suggests that this patch's change might be problematic:
>
>     # on Windows
>     $ pwd
>     /home/me
>     $ pwd -W
>     C:/msys64/home/me
>     $ pwd -P
>     /home/me
>     $ pwd -W -P
>     /home/me

Because pwd emulation we use on Windows ignores -P the updated
caller, pfx with this change would not change the existing
behaviour.

How would one test this situation on Windows, I wonder?  Create a
directory that is pointed at by a symbolic link, and use it as the
test directory (either have the checkout there, or use --root to
have the trash directory there)?

> FOOTNOTES
>
> [*]: In the long run, a better fix would probably be for the tests to
> sanitize the output of the Git command, replacing (via `sed`) the
> actual emitted path with some placeholder, such as "%HOME%" or
> something, and then have the tests look for (`grep` or whatnot)
> needles using that literal placeholder rather than trying to perfectly
> match the path emitted by Git. This approach makes sense since these
> tests are about overall functionality of git-maintenance, not about
> the specific path in which the person happens to be running the tests.

Another approach may be to do a form of chdir that forces the shell
to figure out where it really is upfront at the beginning of a test
script, perhaps inside test-lib.sh which happend before anything
meaningful happens in the test (i.e. "cd -P ." or something).

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Mark Mentovai]

Eric Sunshine wrote:
> However, have you tested this on Windows?

Yes, via the CI: 
https://github.com/markmentovai/git/actions/runs/15217563313.

> I ask because, despite the
> test's name, this and most of the tests in this script, are actually
> run on all platforms, and because `pwd` is overridden by a shell
> function for MinGW on Windows:
>
>    # t/test-lib.sh
>    ...
>    # git sees Windows-style pwd
>    pwd () {
>        builtin pwd -W
>    }

That MinGW fallback pwd ignores arguments, so any pwd in a test regardless 
of whether it's specified as pwd or pwd -P will result in an underlying 
pwd -W. The t7900 test's behavior should not change as a result of this 
patch. If it's succeeding in some MinGW environment before this patch, 
it'll continue to succeed after.

> My quick testing suggests that this patch's change might be problematic:
>
>    # on Windows
>    $ pwd
>    /home/me
>    $ pwd -W
>    C:/msys64/home/me
>    $ pwd -P
>    /home/me
>    $ pwd -W -P
>    /home/me
>
> FOOTNOTES
>
> [*]: In the long run, a better fix would probably be for the tests to
> sanitize the output of the Git command, replacing (via `sed`) the
> actual emitted path with some placeholder, such as "%HOME%" or
> something, and then have the tests look for (`grep` or whatnot)
> needles using that literal placeholder rather than trying to perfectly
> match the path emitted by Git. This approach makes sense since these
> tests are about overall functionality of git-maintenance, not about
> the specific path in which the person happens to be running the tests.

The specific front of the path is not important, but the tail should be as 
expected, and I suspect that it remains much less fragile and complex to 
perform this equality comparison than it would be to try to reason about 
the path's inner components.

The existing print-args in this test could be modified as you propose, but 
the changes would also need to spill into the "start and stop when several 
schedulers are available" test later in the same file. That seems more 
invasive and produces less clear test code than just calculating a path 
expectation in line with what git maintenance uses in the first place.

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Eric Sunshine]

On Fri, May 23, 2025 at 4:42 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> >> -       pfx=$(cd "$HOME" && pwd) &&
> >> +       pfx=$(cd "$HOME" && pwd -P) &&
> >
> > However, have you tested this on Windows? I ask because, despite the
> > test's name, this and most of the tests in this script, are actually
> > run on all platforms, and because `pwd` is overridden by a shell
> > function for MinGW on Windows:
> >
> >     # t/test-lib.sh
> >     ...
> >     # git sees Windows-style pwd
> >     pwd () {
> >         builtin pwd -W
> >     }
>
> Because pwd emulation we use on Windows ignores -P the updated
> caller, pfx with this change would not change the existing
> behaviour.

True. I overlooked that[*].

[*]: This is the second time in two days I made a stupid mistake when
replying to a patch. I should probably stop trying to read/review
patches while my mind is more focused on whatever task I'm actually
working on despite having a few moments available while waiting for a
compilation or other lengthy operation to finish.

> How would one test this situation on Windows, I wonder?  Create a
> directory that is pointed at by a symbolic link, and use it as the
> test directory (either have the checkout there, or use --root to
> have the trash directory there)?

Perhaps(?). The Windows experts on the list are probably better suited
to answer.

> > [*]: In the long run, a better fix would probably be for the tests to
> > sanitize the output of the Git command, replacing (via `sed`) the
> > actual emitted path with some placeholder, such as "%HOME%" or
> > something, and then have the tests look for (`grep` or whatnot)
> > needles using that literal placeholder rather than trying to perfectly
> > match the path emitted by Git. This approach makes sense since these
> > tests are about overall functionality of git-maintenance, not about
> > the specific path in which the person happens to be running the tests.
>
> Another approach may be to do a form of chdir that forces the shell
> to figure out where it really is upfront at the beginning of a test
> script, perhaps inside test-lib.sh which happend before anything
> meaningful happens in the test (i.e. "cd -P ." or something).

Hmm, I'm not sure how that would help this particular case which wants
to know the path of $HOME:

    pfx=$(cd "$HOME" && pwd -P) &&

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Eric Sunshine]

On Fri, May 23, 2025 at 4:43 PM Mark Mentovai <mark@chromium.org> wrote:
> Eric Sunshine wrote:
> >    # t/test-lib.sh
> >    ...
> >    # git sees Windows-style pwd
> >    pwd () {
> >        builtin pwd -W
> >    }
>
> That MinGW fallback pwd ignores arguments, so any pwd in a test regardless
> of whether it's specified as pwd or pwd -P will result in an underlying
> pwd -W. The t7900 test's behavior should not change as a result of this
> patch. If it's succeeding in some MinGW environment before this patch,
> it'll continue to succeed after.

You're right. I (stupidly) overlooked that the `pwd` function ignores
its arguments.

> > [*]: In the long run, a better fix would probably be for the tests to
> > sanitize the output of the Git command, replacing (via `sed`) the
> > actual emitted path with some placeholder, such as "%HOME%" or
> > something, and then have the tests look for (`grep` or whatnot)
> > needles using that literal placeholder rather than trying to perfectly
> > match the path emitted by Git. This approach makes sense since these
> > tests are about overall functionality of git-maintenance, not about
> > the specific path in which the person happens to be running the tests.
>
> The specific front of the path is not important, but the tail should be as
> expected, and I suspect that it remains much less fragile and complex to
> perform this equality comparison than it would be to try to reason about
> the path's inner components.

`sed` in my suggestion was an afterthought. What I really had in mind
was perhaps augmenting the value of the GIT_TEST_MAINT_SCHEDULER
environment variable, which git-maintenance already specially
recognizes to tweak its behavior when being run by a test, such that
it instead emits the placeholder (literal "%HOME%" or whatever) as
prefix of the path rather than the actual prefix. Tests could then
just employ the same placeholder prefix rather than trying to exactly
match the real path prefix.

Such a change is, of course, well outside the scope of the patch under
discussion (hence my phrasing "in the long run"). The minimal fix you
presented should be perfectly satisfactory.

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Hmm, I'm not sure how that would help this particular case which wants
> to know the path of $HOME:
>
>     pfx=$(cd "$HOME" && pwd -P) &&

We set HOME to TRASH_DIRECTORY during test, so HOME would not be all
that special.

I thought one of the issues was on Windows "-P" in "pwd -P" is a
no-op?  So if you want to fix it locally perhaps

    pfx=$(cd -P "$HOME" && pwd)

but I think the true issue may be that we set up fake HOME using
TRASH_DIRECTORY that is before we canonicalize it with "cd -P".

Would doing something like this (without any other changes we
discussed so far) help?

 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git c/t/test-lib.sh w/t/test-lib.sh
index af722d383d..92d0db13d7 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -1577,6 +1577,8 @@ fi
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
+TRASH_DIRECTORY=$(pwd)
+HOME="$TRASH_DIRECTORY"
 
 start_test_output "$0"
 

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Mark Mentovai]

Junio C Hamano wrote:
> Would doing something like this (without any other changes we
> discussed so far) help?
>
> t/test-lib.sh | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git c/t/test-lib.sh w/t/test-lib.sh
> index af722d383d..92d0db13d7 100644
> --- c/t/test-lib.sh
> +++ w/t/test-lib.sh
> @@ -1577,6 +1577,8 @@ fi
> # Use -P to resolve symlinks in our working directory so that the cwd
> # in subprocesses like git equals our $PWD (for pathname comparisons).
> cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
> +TRASH_DIRECTORY=$(pwd)
> +HOME="$TRASH_DIRECTORY"
>
>  start_test_output "$0"
>

That does work as an alternate way to fix the bug that I described.

Forcing the entire test suite to run from a realpath-canonicalized path 
seems like a blunt instrument, though. It would tend to mask bugs that 
only occur when git runs with a non-canonical path as its working 
directory. It could even mask bugs in git's own pathname canonicalization 
code.

Re: [PATCH] t7900: use pwd -P in macOS maintenance test

[Junio C Hamano]

Mark Mentovai <mark@chromium.org> writes:

> Forcing the entire test suite to run from a realpath-canonicalized
> path seems like a blunt instrument, though.

I view it differently.

It is a very effective way to make sure that the tests work for one
person does not mysteriously break for another, only due to where
they kept the git repository and where in their filesystem they
arranged the tests to run.  That is not the kind of breakage we want
our developers to be scratching their heads about.

The part of the test-lib I showed already was trying to make sure
that our idea of $PWD matches the physical world.  It did so only
for PWD, by doing 'cd -P "$TRASH_DIRECTORY"' early in the test setup
sequence, but it did so after setting HOME, leaving it inconsistent
between physical and logical world.

Which is the bug that my suggested fix addresses.

Besides the problem we saw in this discussion thread is *not* a bug
in Git and its normalization of its working directory, is it?  It
was a bug in the test script that assumed that $HOME already was
normalized, right?

If we want to make sure Git (not its tests, but Git itself) works
when (pwd) and (pwd -P) are different, we should have a dedicated
test to arrange that for everybody, regardless of where the tester
checks out their git repository.  We can only do so on symlink
capable filesystems, but we do have enough support in the test
framework to do so if you want to write such a test.

[PATCH v2] t: run tests from a normalized working directory

[Mark Mentovai]

Some tests make git perform actions that produce observable pathnames,
and have expectations on those paths. Tests run with $HOME set to a
$TRASH_DIRECTORY, and with their working directory the same
$TRASH_DIRECTORY, although these paths are logically identical, they do
not observe the same pathname canonicalization rules and thus might not
be represented by strings that compare equal. In particular, no pathname
normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
change their working directory with `cd -P`, which normalizes the
working directory's path by fully resolving symbolic links.

t7900's macOS maintenance tests (which are not limited to running on
macOS) have an expectation on a path that `git maintenance` forms by
using abspath.c strbuf_realpath() to resolve a canonical absolute path
based on $HOME. When t7900 runs from a working directory that contains
symbolic links in its pathname, $HOME will also contain symbolic links,
which `git maintenance` resolves but the test's expectation does not,
causing a test failure.

Align $TRASH_DIRECTORY and $HOME with the normalized path as used for
the working directory by resetting them to match the working directory
after it's established by `cd -P`. With all paths in agreement and
symbolic links resolved, pathname expectations can be set and met based
on string comparison without regard to external environmental factors
such as the presence of symbolic links in a path.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Mark Mentovai <mark@chromium.org>
---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index af722d383d9b..92d0db13d742 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1577,6 +1577,8 @@ fi
 # Use -P to resolve symlinks in our working directory so that the cwd
 # in subprocesses like git equals our $PWD (for pathname comparisons).
 cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
+TRASH_DIRECTORY=$(pwd)
+HOME="$TRASH_DIRECTORY"
 
 start_test_output "$0"
 

Range-diff against v1:
1:  95491c294c1e ! 1:  496e490a6462 t7900: use pwd -P in macOS maintenance test
    @@ Metadata
     Author: Mark Mentovai <mark@chromium.org>
     
      ## Commit message ##
    -    t7900: use pwd -P in macOS maintenance test
    +    t: run tests from a normalized working directory
     
    -    $pfx is the basis for the expectation that launchd plist paths formed by
    -    `git maintenance start` will be compared against. These paths are formed
    -    in `git maintenance` by builtin/gc.c launchctl_service_filename(), which
    -    calls path.c interpolate_path() with real_home = 1, causing abspath.c
    -    strbuf_realpath() to resolve a canonical absolute path. Since $pfx is
    -    not determined according to the same realpath semantics, when t7900 is
    -    run from a working directory that contains a symbolic link in its path,
    -    the realpath operation will produce a different path than $pfx contains,
    -    although both paths logically reference the same directory. The test
    -    fails in this case.
    +    Some tests make git perform actions that produce observable pathnames,
    +    and have expectations on those paths. Tests run with $HOME set to a
    +    $TRASH_DIRECTORY, and with their working directory the same
    +    $TRASH_DIRECTORY, although these paths are logically identical, they do
    +    not observe the same pathname canonicalization rules and thus might not
    +    be represented by strings that compare equal. In particular, no pathname
    +    normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
    +    change their working directory with `cd -P`, which normalizes the
    +    working directory's path by fully resolving symbolic links.
     
    -    Base $pfx on the physical working directory (pwd -P), with all symbolic
    -    links fully resolved, so that the path that the test expects matches
    -    what `git maintenance` generates, even when running from a working
    -    directory whose path contains a symbolic link.
    +    t7900's macOS maintenance tests (which are not limited to running on
    +    macOS) have an expectation on a path that `git maintenance` forms by
    +    using abspath.c strbuf_realpath() to resolve a canonical absolute path
    +    based on $HOME. When t7900 runs from a working directory that contains
    +    symbolic links in its pathname, $HOME will also contain symbolic links,
    +    which `git maintenance` resolves but the test's expectation does not,
    +    causing a test failure.
     
    +    Align $TRASH_DIRECTORY and $HOME with the normalized path as used for
    +    the working directory by resetting them to match the working directory
    +    after it's established by `cd -P`. With all paths in agreement and
    +    symbolic links resolved, pathname expectations can be set and met based
    +    on string comparison without regard to external environmental factors
    +    such as the presence of symbolic links in a path.
    +
    +    Suggested-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Mark Mentovai <mark@chromium.org>
     
    - ## t/t7900-maintenance.sh ##
    -@@ t/t7900-maintenance.sh: test_expect_success 'stop preserves surrounding schedule' '
    + ## t/test-lib.sh ##
    +@@ t/test-lib.sh: fi
    + # Use -P to resolve symlinks in our working directory so that the cwd
    + # in subprocesses like git equals our $PWD (for pathname comparisons).
    + cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
    ++TRASH_DIRECTORY=$(pwd)
    ++HOME="$TRASH_DIRECTORY"
      
    - test_expect_success 'start and stop macOS maintenance' '
    - 	# ensure $HOME can be compared against hook arguments on all platforms
    --	pfx=$(cd "$HOME" && pwd) &&
    -+	pfx=$(cd "$HOME" && pwd -P) &&
    + start_test_output "$0"
      
    - 	write_script print-args <<-\EOF &&
    - 	echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args
-- 
2.49.0

Re: [PATCH v2] t: run tests from a normalized working directory

[Torsten Bögershausen]

On Wed, May 28, 2025 at 04:17:37PM -0400, Mark Mentovai wrote:

The problem is well described, thanks for that.
However, different words and terms are used for the same thing:
  "normalized working directory" (which is easy to confuse
    with normalized working tree where CRLF-LF conversion had been
    done and clean filters applied.
  "pathname canonicalization"
  "canonical absolute path"
  "normalized path"
... and that is done in "strbuf_realpath()"

May be the word normalized can be replaced here ?
Starting with the head line, how about this:
t: run tests from an absolute path

And later in the text:
use "absolute path" instead of "normalized path" ?

> Some tests make git perform actions that produce observable pathnames,
> and have expectations on those paths. Tests run with $HOME set to a
> $TRASH_DIRECTORY, and with their working directory the same
> $TRASH_DIRECTORY, although these paths are logically identical, they do
> not observe the same pathname canonicalization rules and thus might not
> be represented by strings that compare equal. In particular, no pathname
> normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
> change their working directory with `cd -P`, which normalizes the
> working directory's path by fully resolving symbolic links.
> 
> t7900's macOS maintenance tests (which are not limited to running on
> macOS) have an expectation on a path that `git maintenance` forms by
> using abspath.c strbuf_realpath() to resolve a canonical absolute path
> based on $HOME. When t7900 runs from a working directory that contains
> symbolic links in its pathname, $HOME will also contain symbolic links,
> which `git maintenance` resolves but the test's expectation does not,
> causing a test failure.
> 
> Align $TRASH_DIRECTORY and $HOME with the normalized path as used for
> the working directory by resetting them to match the working directory
> after it's established by `cd -P`. With all paths in agreement and
> symbolic links resolved, pathname expectations can be set and met based
> on string comparison without regard to external environmental factors
> such as the presence of symbolic links in a path.
> 
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Mark Mentovai <mark@chromium.org>
> ---
>  t/test-lib.sh | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index af722d383d9b..92d0db13d742 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1577,6 +1577,8 @@ fi
>  # Use -P to resolve symlinks in our working directory so that the cwd
>  # in subprocesses like git equals our $PWD (for pathname comparisons).
>  cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
> +TRASH_DIRECTORY=$(pwd)
> +HOME="$TRASH_DIRECTORY"
>  
>  start_test_output "$0"
>  
> 

Re: [PATCH v2] t: run tests from a normalized working directory

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On Wed, May 28, 2025 at 04:17:37PM -0400, Mark Mentovai wrote:
>
> The problem is well described, thanks for that.
> However, different words and terms are used for the same thing:
>   "normalized working directory" (which is easy to confuse
>     with normalized working tree where CRLF-LF conversion had been
>     done and clean filters applied.
>   "pathname canonicalization"
>   "canonical absolute path"
>   "normalized path"
> ... and that is done in "strbuf_realpath()"
>
> May be the word normalized can be replaced here ?
> Starting with the head line, how about this:
> t: run tests from an absolute path
>
> And later in the text:
> use "absolute path" instead of "normalized path" ?

Thanks for a careful reading.  When I read the log message for the
first time, it did not bother me too much that it said canonicalize
and normalize interchangeably, and the two verbs still do not bother
me terribly.  I agree with you that clearly describing what the
canonicalization rules do (i.e. fully resolving symbolic links to
make the path absolute) very good idea, but I think the last
sentence of the first paragraph does a good job at it already.

Can you offer a rewrite along the lines you suggest so that we can
compare?  I personally felt that what Mark gave us, albeit a bit on
the more verbose and repetitive side, were written clearly enough.

Thanks.

>> Some tests make git perform actions that produce observable pathnames,
>> and have expectations on those paths. Tests run with $HOME set to a
>> $TRASH_DIRECTORY, and with their working directory the same
>> $TRASH_DIRECTORY, although these paths are logically identical, they do
>> not observe the same pathname canonicalization rules and thus might not
>> be represented by strings that compare equal. In particular, no pathname
>> normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
>> change their working directory with `cd -P`, which normalizes the
>> working directory's path by fully resolving symbolic links.
>>
>> t7900's macOS maintenance tests (which are not limited to running on
>> macOS) have an expectation on a path that `git maintenance` forms by
>> using abspath.c strbuf_realpath() to resolve a canonical absolute path
>> based on $HOME. When t7900 runs from a working directory that contains
>> symbolic links in its pathname, $HOME will also contain symbolic links,
>> which `git maintenance` resolves but the test's expectation does not,
>> causing a test failure.
>> 
>> Align $TRASH_DIRECTORY and $HOME with the normalized path as used for
>> the working directory by resetting them to match the working directory
>> after it's established by `cd -P`. With all paths in agreement and
>> symbolic links resolved, pathname expectations can be set and met based
>> on string comparison without regard to external environmental factors
>> such as the presence of symbolic links in a path.
>> 
>> Suggested-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Mark Mentovai <mark@chromium.org>
>> ---
>>  t/test-lib.sh | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index af722d383d9b..92d0db13d742 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1577,6 +1577,8 @@ fi
>>  # Use -P to resolve symlinks in our working directory so that the cwd
>>  # in subprocesses like git equals our $PWD (for pathname comparisons).
>>  cd -P "$TRASH_DIRECTORY" || BAIL_OUT "cannot cd -P to \"$TRASH_DIRECTORY\""
>> +TRASH_DIRECTORY=$(pwd)
>> +HOME="$TRASH_DIRECTORY"
>>  
>>  start_test_output "$0"
>>  
>> 

Re: [PATCH v2] t: run tests from a normalized working directory

[Torsten Bögershausen]

On Thu, May 29, 2025 at 10:04:24PM -0700, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > On Wed, May 28, 2025 at 04:17:37PM -0400, Mark Mentovai wrote:
> >
> > The problem is well described, thanks for that.
> > However, different words and terms are used for the same thing:
> >   "normalized working directory" (which is easy to confuse
> >     with normalized working tree where CRLF-LF conversion had been
> >     done and clean filters applied.
> >   "pathname canonicalization"
> >   "canonical absolute path"
> >   "normalized path"
> > ... and that is done in "strbuf_realpath()"
> >
> > May be the word normalized can be replaced here ?
> > Starting with the head line, how about this:
> > t: run tests from an absolute path
> >
> > And later in the text:
> > use "absolute path" instead of "normalized path" ?
> 
> Thanks for a careful reading.  When I read the log message for the
> first time, it did not bother me too much that it said canonicalize
> and normalize interchangeably, and the two verbs still do not bother
> me terribly.  I agree with you that clearly describing what the
> canonicalization rules do (i.e. fully resolving symbolic links to
> make the path absolute) very good idea, but I think the last
> sentence of the first paragraph does a good job at it already.
> 
> Can you offer a rewrite along the lines you suggest so that we can
> compare?  I personally felt that what Mark gave us, albeit a bit on
> the more verbose and repetitive side, were written clearly enough.
> 
> Thanks.

Here is an attempt to do so.
The word canonical has been removed.
After reading the help for
'pwd -P' and 'cd -P'
"absolute" is replaced by "physical".
A matter of taste.
If absolute is more used here in Git, I am fine with any.

==================================================
t: run 7900 tests from the physical working directory

Some tests make git perform actions that produce observable pathnames,
and have expectations on those paths. Tests run with $HOME set to a
$TRASH_DIRECTORY, and with their working directory the same
$TRASH_DIRECTORY, although these paths are physical identical, they do
not observe the same pathname normalization rules and thus might not
be represented by strings that compare equal. In particular, no pathname
normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
change their working directory with `cd -P` which resolves symbolic links
returning the physical path.

t7900's macOS maintenance tests (which are not limited to running on
macOS) have an expectation on a path that `git maintenance` forms by
using abspath.c strbuf_realpath() to resolve the physical path
based on $HOME. When t7900 runs from a working directory that contains
symbolic links in its pathname, $HOME will also contain symbolic links,
which `git maintenance` resolves but the test's expectation does not,
causing a test failure.

Align $TRASH_DIRECTORY and $HOME with the physical path as used for
the working directory by resetting them to match the working directory
after it's established by `cd -P`. With all paths in agreement and
symbolic links resolved, pathname expectations can be set and met based
on string comparison without regard to external environmental factors
such as the presence of symbolic links in a path.

Re: [PATCH v2] t: run tests from a normalized working directory

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> The word canonical has been removed.
> After reading the help for
> 'pwd -P' and 'cd -P'
> "absolute" is replaced by "physical".
> A matter of taste.
> If absolute is more used here in Git, I am fine with any.

It is OK as long as we are locally consistent.  I do think inside
our codebase it seems we use "absolute" more, but the change in
question is about use of "-P" option, which certainly was taken from
"physical", in our test scripts, so I am OK with your description
below to use that word.

If somebody really cared (and I don't), we may want to pick a single
word among physical, absolute, and real, but the only thing is that
we are using them interchangeably, so as long as we make it clear
(e.g. perhaps strbuf_realpath() and the underlying helper functions
that are used by it may have a comment or two that says that we use
these three words interchangeably to our developers), it would be
good enough.

> ==================================================
> t: run 7900 tests from the physical working directory
>
> Some tests make git perform actions that produce observable pathnames,
> and have expectations on those paths. Tests run with $HOME set to a
> $TRASH_DIRECTORY, and with their working directory the same
> $TRASH_DIRECTORY, although these paths are physical identical, they do
> not observe the same pathname normalization rules and thus might not
> be represented by strings that compare equal.
> In particular, no pathname
> normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
> change their working directory with `cd -P` which resolves symbolic links
> returning the physical path.

"physical identical"?  I think the problem is $HOME and $TRASH are
the same but not physically normalized, which means "cd $HOME &&
pwd", "cd $HOME && pwd -P" and "cd -P $HOME && pwd" can give
different results from these two variables.  How about replacing the
latter half of the above with something much simpler, like this?

    ... although HOME and TRASH_DIRECTORY have identical values, the
    physical path to it (i.e. what "cd $HOME && pwd -P" reports) may
    be different.

> t7900's macOS maintenance tests (which are not limited to running on
> macOS) have an expectation on a path that `git maintenance` forms by
> using abspath.c strbuf_realpath() to resolve the physical path
> based on $HOME. When t7900 runs from a working directory that contains
> symbolic links in its pathname, $HOME will also contain symbolic links,
> which `git maintenance` resolves but the test's expectation does not,
> causing a test failure.
>
> Align $TRASH_DIRECTORY and $HOME with the physical path as used for
> the working directory by resetting them to match the working directory
> after it's established by `cd -P`. With all paths in agreement and
> symbolic links resolved, pathname expectations can be set and met based
> on string comparison without regard to external environmental factors
> such as the presence of symbolic links in a path.

Re: [PATCH v2] t: run tests from a normalized working directory

[Mark Mentovai]

Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> The word canonical has been removed.
>> After reading the help for
>> 'pwd -P' and 'cd -P'
>> "absolute" is replaced by "physical".
>> A matter of taste.
>> If absolute is more used here in Git, I am fine with any.
>
> It is OK as long as we are locally consistent.  I do think inside
> our codebase it seems we use "absolute" more, but the change in
> question is about use of "-P" option, which certainly was taken from
> "physical", in our test scripts, so I am OK with your description
> below to use that word.
>
> If somebody really cared (and I don't), we may want to pick a single
> word among physical, absolute, and real, but the only thing is that
> we are using them interchangeably, so as long as we make it clear
> (e.g. perhaps strbuf_realpath() and the underlying helper functions
> that are used by it may have a comment or two that says that we use
> these three words interchangeably to our developers), it would be
> good enough.

An "absolute" path is well-defined and commonly understood to have a 
singular meaning. These paths are relative to the root directory, and are 
identified by a leading separator (/). POSIX specifies this at XBD.3.2[1] 
and XBD.4.16[2].

This change is not concerned with absolute paths. All of the paths in 
question are absolute, both before and after this change.

There is no single term that's widely understood to have the meaning that 
this change is concerned with. "Physical" is a contender, but this term is 
not broadly understood without providing additional context. In fact, 
while "physical" is the namesake for the pwd's `-P` option, the POSIX 
definition of pwd(1) doesn't even use this term, instead explaining the 
concept fully in prose[3]. I don't believe it's appropriate to discuss 
"physical" paths without an introduction clarifying its intended meaning.

`realpath` is a library interface that transforms paths to those having 
the semantics at issue, but it's somewhat obscure, and easily confused 
with "real path" whose meaning would be entirely ambiguous. realpath(3) 
documentation from POSIX[4] explains the semantics fully; glibc[5], and 
Linux man-pages[6] provide full explanation while also using the term 
"canonicalize".

"Canonicalize" alone is too generic, because there are several axes of 
canonicalization that may apply to a path, some filesystem-dependent. This 
change is concerned with canonicalization via symbolic link resolution, 
but in other contexts, path canonicalization may refer to other concepts, 
such as case matching on case-preserving filesystems, or encoding 
canonicalization (such as Unicode normalization) on filesystems that have 
defined encoding rules.

All of this illustrates the difficulty in choosing a single term to 
unambiguously convey the meaning. I chose to write a commit message that 
favored technical precision, even if it meant tending toward what Junio 
called "the more verbose and repetitive side". I believed that to be 
necessary to fully explain the background, the problem, and the solution.

I don't know that this amount of scrutiny of a commit message that's 
precise and correct is entirely justified, but if there's a strong 
preference to converge on a single term, provided that it's not 
technically inaccurate, I'll accede to the request. I don't expect that it 
will shrink the amount of explanatory text, though, because as I've 
illustrated, there isn't really a term in existence that conveys this 
principle generally in a way that's suitable for broad understanding.

[1] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap03.html#tag_03_02
[2] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap04.html#tag_04_16
[3] https://pubs.opengroup.org/onlinepubs/9799919799/utilities/pwd.html
[4] https://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html
[5] https://sourceware.org/glibc/manual/2.41/html_node/Symbolic-Links.html#index-realpath
[6] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man/man3/realpath.3

>> ==================================================
>> t: run 7900 tests from the physical working directory
>>
>> Some tests make git perform actions that produce observable pathnames,
>> and have expectations on those paths. Tests run with $HOME set to a
>> $TRASH_DIRECTORY, and with their working directory the same
>> $TRASH_DIRECTORY, although these paths are physical identical, they do
>> not observe the same pathname normalization rules and thus might not
>> be represented by strings that compare equal.
>> In particular, no pathname
>> normalization is applied to $TRASH_DIRECTORY or $HOME, while tests
>> change their working directory with `cd -P` which resolves symbolic links
>> returning the physical path.
>
> "physical identical"?  I think the problem is $HOME and $TRASH are
> the same but not physically normalized, which means "cd $HOME &&
> pwd", "cd $HOME && pwd -P" and "cd -P $HOME && pwd" can give
> different results from these two variables.  How about replacing the
> latter half of the above with something much simpler, like this?
>
>    ... although HOME and TRASH_DIRECTORY have identical values, the
>    physical path to it (i.e. what "cd $HOME && pwd -P" reports) may
>    be different.
>
>> t7900's macOS maintenance tests (which are not limited to running on
>> macOS) have an expectation on a path that `git maintenance` forms by
>> using abspath.c strbuf_realpath() to resolve the physical path
>> based on $HOME. When t7900 runs from a working directory that contains
>> symbolic links in its pathname, $HOME will also contain symbolic links,
>> which `git maintenance` resolves but the test's expectation does not,
>> causing a test failure.
>>
>> Align $TRASH_DIRECTORY and $HOME with the physical path as used for
>> the working directory by resetting them to match the working directory
>> after it's established by `cd -P`. With all paths in agreement and
>> symbolic links resolved, pathname expectations can be set and met based
>> on string comparison without regard to external environmental factors
>> such as the presence of symbolic links in a path.
>

Re: [PATCH v2] t: run tests from a normalized working directory

[Junio C Hamano]

Mark Mentovai <mark@chromium.org> writes:

> `realpath` is a library interface that transforms paths to those
> having the semantics at issue, but it's somewhat obscure, and easily
> confused with "real path" whose meaning would be entirely
> ambiguous. realpath(3) documentation from POSIX[4] explains the
> semantics fully; glibc[5], and Linux man-pages[6] provide full
> explanation while also using the term "canonicalize".
>
> "Canonicalize" alone is too generic, because there are several axes of

Yes.  You need to specify what you are canonicalizing to, and once
you are going to do so, there is no need for that heavy verb, i.e.
you do not need to say "canonicalize it to realpath"---you say "turn
it into realpath" and you convey what you want to say just fine.

> All of this illustrates the difficulty in choosing a single term to
> unambiguously convey the meaning. I chose to write a commit message
> that favored technical precision, even if it meant tending toward what
> Junio called "the more verbose and repetitive side". I believed that
> to be necessary to fully explain the background, the problem, and the
> solution.

Yup, that is why I said I thought your original was clear enough.

I am tempted to say that we take what we have from you and merge it
down.

Re: [PATCH v2] t: run tests from a normalized working directory

[Torsten Bögershausen]

On Mon, Jun 02, 2025 at 02:32:35PM -0700, Junio C Hamano wrote:
> Mark Mentovai <mark@chromium.org> writes:
> 
> > `realpath` is a library interface that transforms paths to those
> > having the semantics at issue, but it's somewhat obscure, and easily
> > confused with "real path" whose meaning would be entirely
> > ambiguous. realpath(3) documentation from POSIX[4] explains the
> > semantics fully; glibc[5], and Linux man-pages[6] provide full
> > explanation while also using the term "canonicalize".
> >
> > "Canonicalize" alone is too generic, because there are several axes of
> 
> Yes.  You need to specify what you are canonicalizing to, and once
> you are going to do so, there is no need for that heavy verb, i.e.
> you do not need to say "canonicalize it to realpath"---you say "turn
> it into realpath" and you convey what you want to say just fine.
> 
> > All of this illustrates the difficulty in choosing a single term to
> > unambiguously convey the meaning. I chose to write a commit message
> > that favored technical precision, even if it meant tending toward what
> > Junio called "the more verbose and repetitive side". I believed that
> > to be necessary to fully explain the background, the problem, and the
> > solution.
> 
> Yup, that is why I said I thought your original was clear enough.
> 
> I am tempted to say that we take what we have from you and merge it
> down.
> 

Thanks for the long explanations.
I still stumble across the headline:
t: run tests from a normalized working directory

Re-reading the help for realpath() and pwd, would this makes sense:
t: run tests from an absolute pathname

Re: [PATCH v2] t: run tests from a normalized working directory

[Mark Mentovai]

Torsten Bögershausen wrote:
> On Mon, Jun 02, 2025 at 02:32:35PM -0700, Junio C Hamano wrote:
>> Mark Mentovai <mark@chromium.org> writes:
>>
>>> `realpath` is a library interface that transforms paths to those
>>> having the semantics at issue, but it's somewhat obscure, and easily
>>> confused with "real path" whose meaning would be entirely
>>> ambiguous. realpath(3) documentation from POSIX[4] explains the
>>> semantics fully; glibc[5], and Linux man-pages[6] provide full
>>> explanation while also using the term "canonicalize".
>>>
>>> "Canonicalize" alone is too generic, because there are several axes of
>>
>> Yes.  You need to specify what you are canonicalizing to, and once
>> you are going to do so, there is no need for that heavy verb, i.e.
>> you do not need to say "canonicalize it to realpath"---you say "turn
>> it into realpath" and you convey what you want to say just fine.
>>
>>> All of this illustrates the difficulty in choosing a single term to
>>> unambiguously convey the meaning. I chose to write a commit message
>>> that favored technical precision, even if it meant tending toward what
>>> Junio called "the more verbose and repetitive side". I believed that
>>> to be necessary to fully explain the background, the problem, and the
>>> solution.
>>
>> Yup, that is why I said I thought your original was clear enough.
>>
>> I am tempted to say that we take what we have from you and merge it
>> down.
>>
>
> Thanks for the long explanations.
> I still stumble across the headline:
> t: run tests from a normalized working directory
>
> Re-reading the help for realpath() and pwd, would this makes sense:
> t: run tests from an absolute pathname

No. As I wrote earlier:

> An "absolute" path is well-defined and commonly understood to have a
> singular meaning. These paths are relative to the root directory, and are 
> identified by a leading separator (/). POSIX specifies this at XBD.3.2[1] 
> and XBD.4.16[2].
>
> This change is not concerned with absolute paths. All of the paths in
> question are absolute, both before and after this change.
[...]
> [1] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap03.html#tag_03_02
> [2] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap04.html#tag_04_16

Making a path absolute is a different transformation than what is at issue 
here. You may have been misled by the fact that pwd -P and realpath both 
make paths absolute in addition to performing symbolic link resolution. 
The latter is what's operative here.

As I've explained, the paths in question are already absolute in git's 
test suite today, even without the proposed change. It's not correct to 
summarize the change as making paths absolute, when that's neither 
changing nor the crux of the problem.

Re: [PATCH v2] t: run tests from a normalized working directory

[Junio C Hamano]

Mark Mentovai <mark@chromium.org> writes:

> Torsten Bögershausen wrote:
>> On Mon, Jun 02, 2025 at 02:32:35PM -0700, Junio C Hamano wrote:
>>> ...
>>> Yes.  You need to specify what you are canonicalizing to, and once
>>> you are going to do so, there is no need for that heavy verb, i.e.
>>> you do not need to say "canonicalize it to realpath"---you say "turn
>>> it into realpath" and you convey what you want to say just fine.
>>>
>> Re-reading the help for realpath() and pwd, would this makes sense:
>> t: run tests from an absolute pathname
> ...
> Making a path absolute is a different transformation than what is at
> issue here. You may have been misled by the fact that pwd -P and
> realpath both make paths absolute in addition to performing symbolic
> link resolution. The latter is what's operative here.
>
> As I've explained, the paths in question are already absolute in git's
> test suite today, even without the proposed change. It's not correct
> to summarize the change as making paths absolute, when that's neither
> changing nor the crux of the problem.

Absolutely ;-)

"normalized" does invite "normalize to what standard" question, but
as you mentioned in an earlier post, "realpath" is a bit dense for
those who don't read realpath(3) manual pages, and "symlink-resolved
file path" is quite mouthful even though it might be understandable.

Let me merge it down with the commit title as-is and then cook it in
'next' during the -rc period.

Thanks.