Skip to content
This repository has been archived by the owner on Sep 11, 2020. It is now read-only.

storage/worktree: filesystem, support common directory/linked repositories #1098

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saracen
Copy link
Contributor

@saracen saracen commented Mar 29, 2019

This PR implements main/common directory support for linked working tree repositories created by git worktree add (aka GIT_COMMON_DIR as noted in https://git-scm.com/docs/gitrepository-layout).

To summarize:

  • PlainOpenWithOptions now checks to see whether a file called commondir is in the .git directory. If so, it knows this is a linked/local worktree. If commondir is valid, this main worktree it references is opened and set in filesystem.Options as CommonDir. This setting is passed through to DotGit.

  • DotGit has very few changes, as the CommonDir filesystem takes the place of the regular filesystem (fs), and the regular filesystem is set to localfs. In the instances where CommonDir isn't set, both fs and localfs reference the same filesystem, so normal repositories are unaffected.

  • MainFilesystem() complements Filesystem(). 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 localfs unless they start with refs/, then fs the common directory is used. This filesystem selection is handled with fsFromRefPath.


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).

@saracen
Copy link
Contributor Author

saracen commented Apr 17, 2019

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.

@mcuadros mcuadros requested a review from jfontan April 18, 2019 07:17
@jfontan
Copy link
Contributor

jfontan commented Apr 18, 2019

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!

AlexandreCarlton added a commit to AlexandreCarlton/go-git that referenced this pull request Feb 2, 2020
This supports worktrees; none of this code is my own.
@sbourlon
Copy link

Hi @saracen, thanks for the PR. Could you share your fixtures?

@saracen
Copy link
Contributor Author

saracen commented Feb 27, 2020

@sbourlon Apologies, I'd forgotten about this. I've sent a PR to the fixtures repo.

As per @jfontan 's comment, I should probably swap the storage returned from Filesystem() and MainFilesystem() too.

@sbourlon
Copy link

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 [email protected]: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 [email protected]: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 [email protected]: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

@sbourlon
Copy link

sbourlon commented Feb 27, 2020

@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

@saracen
Copy link
Contributor Author

saracen commented Feb 27, 2020

@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
Copy link

thanks @saracen, doing now.

@sbourlon
Copy link

@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?

@sbourlon
Copy link

sbourlon commented Feb 27, 2020

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?

@saracen
Copy link
Contributor Author

saracen commented Feb 27, 2020

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
Copy link

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.

@saracen
Copy link
Contributor Author

saracen commented Feb 27, 2020

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.

@sbourlon
Copy link

sbourlon commented Feb 28, 2020

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
Copy link

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.

@sbourlon
Copy link

sbourlon commented Feb 28, 2020

@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?

@saracen
Copy link
Contributor Author

saracen commented Feb 28, 2020

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
Copy link

@saracen that works for me 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants