Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 8, 2024, 23:32 UTC
Message-ID: <xmqqy1ase1vo.fsf@gitster.g>
In-Reply-To: <CAO_smVjrKJeKr7QgQWryZRErStFk=Y+1T=dwrR_boXQD_X9_Mg@mail.gmail.com>

Kyle Lippincott <spectral@google.com> writes:

>>     $ cat ../seconary/.git
>
> Nit: typo, should be `secondary` (missing the `d`)

Good eyes.  Thanks.

Show 11 quoted lines

>> +       /*
>> +        * We should be a subdirectory of /.git/worktrees inside
>> +        * the $GIT_DIR of the primary worktree.
>> +        *
>> +        * NEEDSWORK: some folks create secondary worktrees out of a
>> +        * bare repository; they don't count ;-), at least not yet.
>> +        */
>> +       if (!strstr(path, "/.git/worktrees/"))
>
> Do we need to be concerned about path separators being different on
> Windows? Or have they already been normalized here?

I am not certain offhand, but if they need to tolerate different separators, they can send in patches.

Show 9 quoted lines

>> +               goto out;
>> +
>> +       /*
>> +        * Does gitdir that points at the ".git" file at the root of
>> +        * the secondary worktree roundtrip here?
>> +        */
>
> What loss of security do we have if we don't have as stringent of a
> check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?

No loss of security.

These "keep result the status we want to return if we want to return immediately here, and always jump to the out label instead of returning" patterns is mere a disciplined way to make it easier to write code that does not leak. The very first one may not have to do the "goto out" and instead immediately return, but by writing this way, I do not need to be always looking out to shoot down patches that adds new check and/or allocations before this condition and early "out".

> Or maybe we even combine the existing ends_with(.git) check with this

At the mechanical level, perhaps, but I'd want logically separate things treated as distinct cases. One is about being inside $GIT_DIR of the primary worktrees (where more than majority of users will encounter) and the new one is about being inside $GIT_DIR of secondaries.