worktree.Status() not respecting .gitignore

[acumino]

When we have multi nested directory and the .gitignore file is inside that. The wroktree.Status() is not respecting patterns inside .gitignore that can cause the git status to dirty.

How to reproduce

Check this repo https://github.com/acumino/git-status. When we run $ go run ./test.go which is basically checking the status and returning an error if status is not clean.

wt, err := repo.Worktree()
	if err != nil {
		fmt.Printf("failed to get git worktree: %v", err)
	}
if stat, err := wt.Status(); err != nil {
		fmt.Printf("failed to get git status: %v", err)
	} else if !stat.IsClean() {
		fmt.Printf("%v\n%v", err, stat)
	}

It throws an error and ideally it should not.

?? pp/hh/hack1/configs/authwebhook.yaml

Explanation

The issue is happening because append() is being used for the slice in recursion at line 64.

go-git/plumbing/format/gitignore/dir.go

Lines 49 to 76 in c785af3

	func ReadPatterns(fs billy.Filesystem, path []string) (ps []Pattern, err error) {
	ps, _ = readIgnoreFile(fs, path, infoExcludeFile)
	subps, _ := readIgnoreFile(fs, path, gitignoreFile)
	ps = append(ps, subps...)
	var fis []os.FileInfo
	fis, err = fs.ReadDir(fs.Join(path...))
	if err != nil {
	return
	}
	for _, fi := range fis {
	if fi.IsDir() && fi.Name() != gitDir {
	var subps []Pattern
	subps, err = ReadPatterns(fs, append(path, fi.Name()))
	if err != nil {
	return
	}
	if len(subps) > 0 {
	ps = append(ps, subps...)
	}
	}
	}
	return
	}

The passed value of fs is being changed with appends and is being used as domain here

go-git/plumbing/format/gitignore/pattern.go

Line 42 in c785af3

res := pattern{domain: domain}

So when this fs is updated next time with append it is causing overwrite to previously written domain values leading to previously added pattern's domain to overwritten. So git status basically at the end store wrong pattern to ignore, leading to dirty git status.

An explanation of how slices are passed in function and are used internally golang https://stackoverflow.com/questions/39993688/are-slices-passed-by-value.

Solution

Instead to passing append(path, fi.Name()) in ReadPatterns() every times a new copy of slice should be passed.

[acumino]

cc - @mcuadros

[sourcec0de]

I was trying to follow along here because I had a similar theory.
https://go.dev/play/p/pjtX_coRiQ8

package main

import "fmt"

func recurse(a []string) []string {
	if len(a) == 3 {
		return a
	}

	return recurse(append(a, "test"))
}

func main() {
	a := []string{"a"}
	b := recurse(a)
	fmt.Println(a, b)
}

My assumption was that because Go slices are pass-by-value that a and b would have the same contents.
They do not in this case. This program evaluated to.

[a] [a test test]

Program exited.

The return value of append will be a slice that points to the newly allocated array.
I could be widely misunderstanding the issue though.

[acumino]

Please check this issue golang/go#52152 for examples.

[acumino]

As suggested in the issue golang/go#52152 there is nothing that can be done from the go side.
If needed I can open a PR with a possible workaround.

[TopherIsSwell]

@acumino , I have no association with this repo, but I think a PR would be welcome. Do you have a fork with your copy-based workaround?

I've run into the issue where there's a large directory (~300M of contents) that is in the .gitignore file. The worktree.Status() call takes anywhere from 1.6s (with repeated calls so that the kernel is caching the directory contents I'm guessing), to 15s,
whereas if I delete that directory, the call takes ~300ms (with repeated calls).

For comparison, the git status command returns in <20ms

I'm guessing respecting the .gitignore would be a non-trivial performance improvement.

[tisonkun]

@acumino another issue I spot on .gitignore is that a global .gitignore doesn't take effect.

[jghal]

I think I'm seeing the same as @tisonkun, with my attempts at debugging reported here. Copying my gitignore file into the directory that I've specified as the Path in my Add/AddOptions doesn't seem to have any impact though.

[mieubrisse]

as @tisonkun mentioned, we're seeing the same issue in #597