On Fri, Mar 8, 2024 at 3:32 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Kyle Lippincott <spectral@xxxxxxxxxx> writes:
>
> >> $ cat ../seconary/.git
> >
> > Nit: typo, should be `secondary` (missing the `d`)
>
> Good eyes. Thanks.
>
> >> + /*
> >> + * 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.
>
> >> + 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.
Then should we just do that?
+ /* Assumption: `path` points to the root of a $GIT_DIR. */
static int is_repo_with_working_tree(const char *path)
{
- return ends_with_path_components(path, ".git");
+ /* $GIT_DIR immediately below the primary working tree */
+ if (ends_with_path_components(path, ".git"))
+ return 1;
+
+ /*
+ * Also allow $GIT_DIRs in secondary worktrees.
+ * These do not end in .git, but are still considered safe because
+ * of the .git component in the path.
+ */
+ if (strstr(path, "/.git/worktrees/"))
+ return 1;
+
+ return 0;
}
>
> 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.
