Improve repository detection and deal with safe.bareRepository=explicit
#117
Conversation
The name `gitDir` is less ambiguous. Also rename method `Path()` to `GitDir()`.
git/git.go
Outdated
| @@ -71,17 +72,36 @@ func NewRepository(path string) (*Repository, error) { | |||
| gitBin: gitBin, | |||
| } | |||
|
|
|||
| full, err := repo.IsFull() | |||
| if err != nil { | |||
| return nil, fmt.Errorf("determining whether the repository is a full clone: %w", err) | |||
There was a problem hiding this comment.
Having worked on a partial clone just today, I misinterpreted the term "full clone" to mean "non-partial". So maybe "full" is a bit ambiguous and should be called "non-shallow" instead?
There was a problem hiding this comment.
That brings up a good question...should git-sizer be willing to operate on a partial clone?
The reason that it can't operate on a shallow clone is obvious...the objects that it needs are simply not there.
With a partial clone, I'm not certain what would happen. Would it fail because some objects are missing? Would those objects be pulled down "on demand"? If the latter, that would almost certainly be an unwelcome surprise. Either way, it should probably refuse to run, don't you think?
That's part of the reason that I called this method IsFull(), because I suspected that we might want to add some other criteria here.
There was a problem hiding this comment.
I'll leave this for now until we decide about partial clones. Oh and by the way, how does one detect that a repo is a partial clone?
There was a problem hiding this comment.
The reason that it can't operate on a shallow clone is obvious...the objects that it needs are simply not there.
Well, from a naive point of view I would ask the question: why can't git-sizer work on the objects that are there, then, and ignore those that aren't there? The same would apply to partial clones, of course. It would make it possible to answer the question I hear a lot: "why on this burning planet is my local repository so large when I specifically used a partial clone to prevent it from becoming so large?".
git_sizer_test.go
Outdated
| func TestMain(m *testing.M) { | ||
| // Allow Git to access the bare repositories of these tests, even if | ||
| // the user configured `safe.bareRepository=explicit` globally. | ||
| os.Setenv("GIT_CONFIG_PARAMETERS", "'safe.bareRepository=all'") |
There was a problem hiding this comment.
This is not needed anymore, as the only time we were needing it was for the git -C <path> rev-parse --git-dir and the cd <path> && git rev-parse --git-path shallow calls, which are no longer called from the test suite for bare repositories.
This commit should therefore be dropped.
There was a problem hiding this comment.
Oh, that's a surprise. The test code does create non-bare repositories, and is capable of processing them, but I guess it never does.
Maybe it should, to make sure that it works.
There was a problem hiding this comment.
The test code does create non-bare repositories, and everything (now) works as expected: the git -C <path> rev-parse --git-dir call works without any hitch there:
Line 82 in fb78b41
The problem I tried to paper over in e62a41c was that bare repositories have a problem with that call if safe.bareRepository=explicit (which really should be the default, and I hope it will become the default soon).
I double-checked that that e62a41c could be dropped, but that's not so: the git-sizer invocations do not go through repo.GitCommand() and therefore have to set GIT_DIR explicitly, too. I did that in fb78b41.
Add a method `Repository.GitPath(relPath)`, which invokes `git rev-parse --git-path $relPath` to find the path to a file within the Git repository. In `NewRepository()`, instantiate the `Repository` object earlier so that the new method can be used to find the path to `shallow`.
Extract a method to determine whether the repository seems to be a full clone. Call it from `NewRepository()`.
If you already have the desired `GIT_DIR`, there's no need to determine it from the current path.
There's no need to deduce the `GIT_DIR` for a bare repository.
mhagger
force-pushed
the
improve-repository-detection
branch
from
a5da580
to
e62a41c
Compare
As of Git v2.38.0, there is an option to prevent Git from accessing bare repositories unless asked for explicitly (via `--git-dir` or `GIT_DIR`): `safe.bareRepository`. The tests of `git sizer`, however, assume that Git will access a bare repository when the current directory points inside that repository. This only works if `safe.bareRepository` indicates that this is safe. If that is not the case, i.e. if `safe.bareRepository` is set to `explicit`, Git demands that the environment variable `GIT_DIR` is set (either explicitly, or via `--git-dir`) when accessing bare repositories. So let's set `GIT_DIR` for the test cases that work on bare repositories. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
improve-repository-detection
branch
from
e62a41c
to
fb78b41
Compare
When reviewing #116 I noticed that repository detection was pretty confusing overall:
pathrepresents in various places (path where command is run, orGIT_PATH)git rev-parse --git-pathresult depends subtly on how the command is runNewRepository()NewRepository()always tries to deduce theGIT_DIR, but that is unnecessary for bare repositoriesAnd that's in addition to the problem of dealing with
safe.bareRepository=explicit, as explained in #116, which is also made more awkward by the fact thatgit rev-parse --git-path shallowwas being invoked ad-hoc instead of through aRepositoryobject.This PR is an attempt to make things clearer. The last commit is a fix for the tests, swiped from #116 (the other commit from that PR is no longer necessary).
/cc @dscho, @elhmn