Thanks Elijah and Junio,

I appreciate your reviews.  I agree with all of your suggestions. I'll
send a revised patch that incorporates the suggested changes shortly.

Cheers,
Kevin


On Fri, 2022-05-20 at 17:14 -0700, Elijah Newren wrote:

On Thu, May 19, 2022 at 4:39 PM Kevin Locke [off-list ref] wrote:

quoted

    git init repo
    mkdir -p a/b
    cd a/b
    chmod u-x ..
    git -C "${PWD%/a/b}/repo" status

If this example seems a bit contrived, consider running with the
repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))
without ensuring the working directory is accessible by that user.

The code added by e6f8861bd4 to preserve the working directory attempts

When referencing commits in commit messages, this project prefers that the first
reference in the commit message use the output from `git log --no-walk
--pretty=reference $HASH` rather than just $HASH.
So, here, it'd be

    The code added by e6f8861bd4 (setup: introduce
startup_info->original_cwd, 2021-12-09) to preserve the ...

quoted

to normalize the path using strbuf_realpath().  If that fails, as in the
case above, it is treated as a fatal error.  To avoid this, we can
continue after the error.  At worst, git will fail to detect that the
working directory is inside the worktree, resulting in the pre-2.35.0
behavior of not preserving the working directory.

Fixes: e6f8861bd4 ("setup: introduce startup_info->original_cwd")

I was slightly surprised to see this tag, but it appears others in
git.git have used it, so it must just be me that's not familiar with
it.

quoted

Signed-off-by: Kevin Locke <redacted>

Nicely explained commit message.

quoted

---
 setup.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index a7b36f3ffb..fb68caaae0 100644
--- a/setup.c
+++ b/setup.c

@@ -458,11 +458,13 @@ static void setup_original_cwd(void)
         *     not startup_info->original_cwd.
         */

-       /* Normalize the directory */
-       strbuf_realpath(&tmp, tmp_original_cwd, 1);
-       free((char*)tmp_original_cwd);
+       /* Try to normalize the directory.  Fails if ancestor not readable. */

Is that the only reason it fails?  I'm unsure if the second half of
the comment helps there.

quoted

+       if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
+               free((char*)tmp_original_cwd);
+               startup_info->original_cwd = strbuf_detach(&tmp, NULL);
+       } else

git.git coding style: if either of the if/else blocks use braces, use
braces for both

quoted

+               startup_info->original_cwd = tmp_original_cwd;

tmp_original_cwd is not required to be normalized, and there are very
strong normalization assumptions on startup_info->original_cwd.  While
a non-normalized value would work to get pre-2.35.0 behavior, it's by
accident rather than design, and might be confusing for others to
later reason about.  Also, I think it might be possible for
tmp_original_cwd to still be NULL, and some of the immediately
following code I believe will assume it's operating with a non-NULL
value, so you'd need to skip that stuff.  I think the else block here
should use "goto no_prevention_needed", as the no_prevention_needed
block will handle setting startup_info->original_cwd to NULL for you,
and get you the pre-2.35.0 behavior.

quoted

        tmp_original_cwd = NULL;

After changing the above else block to a goto, you may also want to
copy this to the no_prevention_needed block or else copy it to the
else portion of the block above (just before the goto you add).