-
Couldn't load subscription status.
- Fork 534
storage/worktree: filesystem, support common directory/linked repositories #1098
Conversation
27a9f5b to
3932973
Compare
...ories. Signed-off-by: Arran Walker <arran.walker@fiveturns.org>
At the moment I have Filesystem() (returns the local .git filesystem) and MainFilesystem() (returns the common/main .git filesystem).
An alternative would be Filesystem() (returning the main .git filesystem) and a LocalFilesystem() for the local version.
Any preference/thoughts on what Filesystem() should return? I figured most users would expect it to be the local .git directory, and whilst this makes sense, I'm wondering if it will lead to more instances of code not working simply because somebody is in an associated worktree. For example, if existing code somewhere uses Filesystem().Open('hooks/post-checkout')... that would no longer work, as hooks found in the main directory.
I'm starting to think it's more likely developers would expect Filesystem() to use the common directory.
From what I understand most of the files reside in the common .git filesystem so I would use the second alternative (Filesystem and LocalFilesystem). We currently use Filesystem to do some shortcuts like copying packfiles directly to the .git directory instead of sending the objects. If I understand correctly these changes should be done to the common .git directory.
A PR to fixtures with a repository in this fashion would be great for testing.
Thanks!
This supports worktrees; none of this code is my own.
sbourlon
commented
Feb 27, 2020
Hi @saracen, thanks for the PR. Could you share your fixtures?
sbourlon
commented
Feb 27, 2020
thank you @saracen
I am available to test any change if you need.
So far, what I did:
# clone my fork of go-git
GO_GIT_PATH="$HOME/Documents/src/git/github.com/sbourlon/go-git"
git clone git@github.com:sbourlon/go-git.git "$GO_GIT_PATH"
pushd "$GO_GIT_PATH"
git checkout -b common-linked-repo-rebased origin/master
git remote add pr-1098 git@github.com:saracen/go-git.git
git fetch --all
git cherry-pick 39329736aa9fd5cfd4d92e2f8e89207aae739d0b
# fix the tiny conflict
git cherry-pick --continue
# replace the go git fixtures with yours
FIXTURES_PATH="$HOME/Documents/src/git/github.com/saracen/go-git-fixtures"
git clone git@github.com:saracen/go-git-fixtures.git "$FIXTURES_PATH"
git --git-dir=$FIXTURES_PATH/.git checkout linked-worktree
go mod edit -replace gopkg.in/src-d/go-git-fixtures.v3=$FIXTURES_PATH
# because go is complaining that there is no go.mod
pushd $FIXTURES_PATH
go mod init go-git-fixtures
popd
# because the fixtures are still looking for the data/worktree-6e2f6a44e71cdf5f0895d37ea433937e58c051c0.tgz in $GOPATH/src/gopkg.in/src-d/go-git-fixtures.v3
git --git-dir=$GOPATH/src/gopkg.in/src-d/go-git-fixtures.v3/.git pull origin pull/21/head
# start the test
# (do you know how to just run the test "TestLinkedWorktree"?)
go test .
Output:
----------------------------------------------------------------------
FAIL: worktree_test.go:2010: WorktreeSuite.TestLinkedWorktree
worktree_test.go:2038:
c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"repository does not exist"} ("repository does not exist")
OOPS: 262 passed, 1 FAILED
--- FAIL: Test (36.53s)
FAIL
FAIL gopkg.in/src-d/go-git.v4 37.757s
FAIL
@saracen I sent you a PR fixing the gitdir of worktrees (saracen/go-git-fixtures#1). Now the tests are passing:
% go test .
ok gopkg.in/src-d/go-git.v4 34.107s
@sbourlon Thanks for testing and fixing that. I've merged your MR and renamed the filename of the fixture (sha-1 hash of the uncompressed tar).
Are you able to test with the updated fixtures?
sbourlon
commented
Feb 27, 2020
thanks @saracen, doing now.
sbourlon
commented
Feb 27, 2020
@saracen the tests pass, however it is not working in my case because I am using the Open function instead of PlainOpen. Would it be possible to read the dotGitCommonDirectory from Open() instead of PlainOpenWithOptions() https://github.com/src-d/go-git/pull/1098/files#diff-ab2c7209d94fb144c8d55246ad821816R256-R262 so that the implementation covers all the Open functions?
My use case is to run the same command as: git --git-dir=../remote-worktree/.git commit
What about updating NewStorageWithOptions() (https://github.com/src-d/go-git/blob/master/storage/filesystem/storage.go#L46) to detect the commondir from the fs argument?
Are you not able to just use git.PlainOpen("../remote-worktree").
Using that, and committing to the worktree returned from it looks like it might be enough. Sorry if I'm misunderstanding something, I wasn't super knowledgeable about the worktree workflow even when I wrote this almost a year ago :(
sbourlon
commented
Feb 27, 2020
I need to pass both the work tree dir and the gitdir because they are located in different directories. Here is an interesting article explaining that: https://www.logicbig.com/tutorials/misc/git/custom-git-dir.html
In the git cli, that would be: git --git-dir=../remote-worktree/.git --work-tree=. status. In go git, I believe that would be the Open() function.
I need to pass both the work tree dir and the gitdir because they are located in different directories.
PlainOpen supports that though, as far as I understand. If you point it to a worktree directory, containing a .git file (rather than directory), that file contains a gitdir key (https://github.com/src-d/go-git/blob/master/repository.go#L316) that points to the main git repository.
I'm not sure why you'd ever have to provide the gitdir and worktree directory paths up front, as the .git file already links them with that gitdir reference.
thanks @saracen, I was able to reset my linked worktree to the main worktree, however I think PlainOpen() sets the worktree to the linked worktree's worktree, which is different than git --git-dir=../remote-worktree/.git --work-tree=. status.
in my test:
% tree
.
├── lab
│ └── a
└── main
└── a
lab a linked worktree of main and main has the file a modified that I want to commit into lab. The goal is to propagate the changes of main into lab without impacting the git dir of main. With the git cli:
cd main
# status of main (a modified)
git status --short
M a
# status of the linked worktree (not change)
git --git-dir=../lab/.git --work-tree=../lab status --short
(empty)
# status of main compared to the linked worktree (a modified)
git --git-dir=../lab/.git --work-tree=. status --short
M a
# commit the main worktree into lab
git --git-dir=../lab/.git --work-tree=. commit -m "commit in lab" a
[master-stefan 2a66f84] commit in lab
1 file changed, 1 insertion(+)
# status of main (a modified)
git status --short
M a
# status of main compared to the linked worktree (no change)
git --git-dir=../lab/.git status --short
(empty)
I think your solution works with PlainOpenWithOptions() and PlainOpen() but not with Open() because the magic seems to be done in PlainOpenWithOptions() instead of Open().
sbourlon
commented
Feb 28, 2020
it's finally working! I will sent you a small PR to load a separate worktree tomorrow morning (PST time)
Thanks again @saracen for your help.
@saracen here is the branch with your branch rebased on master and my commit that let PlainOpenWithOptions() open a repository with a worktree in a separate directory. I have used this mode to call Status() and Log() successfully.
https://github.com/sbourlon/go-git/commits/common-linked-repo-rebased
Maybe you could:
- get the fixtures merge
- reset your branch on top of mine to make the ci/cd pass
- start the review process?
Hi @sbourlon - I'm not entirely sure what you're doing isn't already supported still. But, it occurs to me that you're wanting the working tree main directory, rather than the dotgit directory. This PR is mostly about dealing with the dotgit side of things and dealing with references in both the common and local repository.
It might be best that I try and work with @jfontan to get this merged soon, and then you create another PR to address your requirements. I know it's a longer process, but probably cleaner than have me be a middleman into what it is you want to achieve. I hope that makes sense.
@jfontan Can you take a look at the fixtures I've submitted and see if they're acceptable? As suggested, I'll modify the method names to Filesystem() and LocalFilesystem() (or maybe WorktreeFilesystem() with a comment explaining that it's the dotgit worktree filesystem?). Is there anything else you'd like done?
sbourlon
commented
Feb 28, 2020
@saracen that works for me 👍
Uh oh!
There was an error while loading. Please reload this page.
This PR implements main/common directory support for linked working tree repositories created by
git worktree add(akaGIT_COMMON_DIRas noted in https://git-scm.com/docs/gitrepository-layout).To summarize:
PlainOpenWithOptionsnow checks to see whether a file calledcommondiris in the .git directory. If so, it knows this is a linked/local worktree. Ifcommondiris valid, this main worktree it references is opened and set infilesystem.OptionsasCommonDir. This setting is passed through toDotGit.DotGithas very few changes, as theCommonDirfilesystem takes the place of the regular filesystem (fs), and the regular filesystem is set tolocalfs. In the instances whereCommonDirisn't set, bothfsandlocalfsreference the same filesystem, so normal repositories are unaffected.MainFilesystem()complementsFilesystem(). They'll return the same underlying filesystem for a normal/non-linked repository.The largest change is the way that pseudo references are handled, mainly "HEAD". All references are read from
localfsunless they start withrefs/, thenfsthe common directory is used. This filesystem selection is handled withfsFromRefPath.Is this the correct approach to this? Any thoughts on additional tests that should be added (I can send a PR to the fixtures repository for what I have so far).