Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix submodule lfsconfig #963

Merged
merged 3 commits into from Feb 2, 2016
Merged

Fix submodule lfsconfig #963

merged 3 commits into from Feb 2, 2016

Conversation

technoweenie
Copy link
Contributor

This is another approach to fixing #922.

While #927 fixes the bug, I'm not completey happy with it. Calling git.RootDir() in loadGitConfig() adds another os exec that's run on every lfs command. This is in addition to when it's run inside resolveGitDir() if the GIT_DIR is specified. I'd like to fix the behavior without running git.RootDir() again.

The problem: git submodule update --init --remote runs git lfs smudge commands with the following env:

GIT_DIR=/path/to/repo/.git/modules/sub
GIT_WORK_TREE=.

When run with the above ENV vars, LocalWorkingDirectory is /path/to/repo/.git/modules, which is wrong. It should be /path/to/repo/sub, where sub is the path for the submodule. Here's the code:

func processWorkTreeVar(gitDir, workTree string) (string, string, error) {
    // See `core.worktree` in `man git-config`:
    // “The value [of core.worktree, GIT_WORK_TREE, or --work-tree] can be an absolute path
    // or relative to the path to the .git directory, which is either specified
    // by --git-dir or GIT_DIR, or automatically discovered.”

    if filepath.IsAbs(workTree) {
        return workTree, gitDir, nil
    }

    base := filepath.Dir(filepath.Clean(gitDir))
    absWorkTree := filepath.Join(base, workTree)
    return absWorkTree, gitDir, nil
}

If we treat GIT_WORK_TREE=. relative to the process' current working directory, it works just fine. It seems like the comments (which are taken directly from the git docs) are wrong in this case. I can pass both --git-dir and --work-tree to get the status of my local git-lfs repo, from a totally different directory:

~/test $ git --git-dir=/Users/rick/p/git-lfs/.git --work-tree="/Users/rick/p/git-lfs" status -sb
## rikdev-fix-submodule-lfsconfig...origin/rikdev-fix-submodule-lfsconfig
 M lfs/config.go
 M lfs/lfs.go
 M test/test-submodule-lfsconfig.sh

If I try relative to the .git directory, it doesn't work.

~/test $ git --git-dir=/Users/rick/p/git-lfs/.git --work-tree=".." status -sb
## rikdev-fix-submodule-lfsconfig...origin/rikdev-fix-submodule-lfsconfig
 D ../.gitattributes

If I try relative to the current working directory, while in the git-lfs directory, it works:

~p/git-lfs git:(rikdev-fix-submodule-lfsconfig) $ git --git-dir=/Users/rick/p/git-lfs/.git --work-tree="." status -sb
## rikdev-fix-submodule-lfsconfig...origin/rikdev-fix-submodule-lfsconfig
 M lfs/config.go
 M lfs/lfs.go
 M test/test-submodule-lfsconfig.sh

@sinbad @rikdev: Thoughts?

@sinbad
Copy link
Contributor

sinbad commented Feb 1, 2016

In #692 I suggested that a future extension was to remove the manual worktree processing entirely and just use git rev-parse --show-toplevel in all cases instead (exposed via git.RootDir()). Would this resolve it?

@technoweenie
Copy link
Contributor Author

I like that idea. I'm playing with a change that calls git rev-parse --git-dir --show-toplevel only. This cuts a lot of code, only runs one exec, and should respect any git env or config values.

Not too many failures. Though the bare repo one is interesting...

test: env with bare repo ...                                       FAILED
test: install updates repo hooks ...                               FAILED
test: submodule env ...                                            FAILED
test: update ...                                                   FAILED

@technoweenie
Copy link
Contributor Author

Cool, got it working. How's this look?

@sinbad
Copy link
Contributor

sinbad commented Feb 2, 2016

Awesome, I didn't know you could use both options on the same command. Looks great, much simpler, more efficient and fixes that bug. 🤘

@technoweenie
Copy link
Contributor Author

I didn't either, just tried it and was pleasantly surprised :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants