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 <kevin@xxxxxxxxxxxxxxx> wrote:
>> 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 ...
>
>> 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.
>
>> Signed-off-by: Kevin Locke <kevin@xxxxxxxxxxxxxxx>
>
> Nicely explained commit message.
>
>> ---
>> 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.
>
>> + 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
>
>> + 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.
>
>> 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).
