Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking

From: Michael McClimon <michael@mcclimon.org>
Date: Oct 22, 2022, 23:19 UTC
Message-ID: <Y1R6ecMO+6X8tFlF@newk>
In-Reply-To: <Y1Rp+7R7e+LFa5k6@coredump.intra.peff.net>

Show 5 quoted lines

> I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix
> branch. That's not strictly necessary, since my patch deletes the line
> you fixed. :) But I think it's nicer to use your fix as the starting
> point, since it means the test runs but produces the wrong behavior,
> rather than barfing with a syntax error.

My vanity thanks you for this, even if it's not strictly necessary. As a professional programmer with roughly no C chops and a long-time admirer of the Git project, all I _really_ wanted to do was to fix a thing that was in my wheelhouse so that I could say I have a commit in the history. (This isn't a good reason on its own, of course, but I'm happy it was useful even if the line is immediately deleted!)

> We can fix this by just relying on rev-parse to tell us when we're not
> in a repository, which fixes the vulnerability. Furthermore, we'll ask
> its --is-bare-repository function to tell us if we're bare or not, and
> rely on that.

Your suggested patch seems fine to me, and indeed I think if we were writing it today we'd just rely on rev-parse to do the heavy lifting. It looks like the code in question -- and indeed, the syntax error in question -- blames to d5c7721d (Git.pm: Add support for subdirectories inside of working copies, 2006-06-23), at which point rev-parse did not appear to have any special handling for bare repositories.

-- 
Michael McClimon
michael@mcclimon.org