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.

Sign up for GitHub

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

Jump to bottom

git: worktree, Fix file reported as Untracked while it is committed #1023

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

git: worktree, Fix file reported as Untracked while it is committed #1023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion worktree_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,45 @@ func (w *Worktree) Status() (Status, error) {
return w.status(hash)
}

func (w *Worktree) newStatusFromIndex() (Status, error) {
idx, err := w.r.Storer.Index()
if err != nil {
return nil, err
}

idxRoot := mindex.NewRootNode(idx)
nodes := []noder.Noder{idxRoot}

if err != nil {
return nil, err
}

status := make(Status)

for len(nodes) > 0 {
var node noder.Noder
node, nodes = nodes[0], nodes[1:]
if node.IsDir() {
children, err := node.Children()
if err != nil {
return nil, err
}
nodes = append(nodes, children...)
continue
}
fs := status.File(node.Name())
fs.Worktree = Unmodified
fs.Staging = Unmodified
}
Comment on lines +65 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have a performance impact, as we need to go through all nodes while growing nodes as part of that process.

What if instead, we amended File(path string) to validate the returning s[path] and force a default value of Unmodified if no value was currently set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't understand what you proposed. Can you explain it a little more?

Copy link
Member

@pjbgf pjbgf Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that going through the entire worktree to create an initial state as unmodified can be quite costly - specially on large repositories. I tested this on a mid-sized repository and noticed that with these changes the .Status() call took ~30% longer than before.

My initial suggestion was to amend Status so that the File func would effectively do the same thing as the pre-init you created. However, this won't really work in some cases, as when there are no changes the Status returns an empty list. Besides, the existing logic is incorrect as whatever file we pass on to status.File(path) it returns Untracked even if that file does not exist across both the staging and the worktree.

In order to fix this, while keeping backwards compatibility and not introducing a performance regression, I think we have a few options:

  1. Introduce a new Worktree.StatusWithOptions() func which returns a new type that is more flexible and calculates the status of a given file on demand. I suspect this will need to have access to the worktree fs, so that it can handle the scenario I mentioned above, without needing to front-load the entire worktree. It may also need access to the exclusion rules.
  2. Introduce a new Worktree.StatusWithOptions() func which returns a new type that is more flexible, but simply adds an option so users can opt-in to the front-loading this PR currently introduces.
  3. Introduce a new Worktree.FileStatus() func which calls .Status and ensure the given file is correctly loaded to the Status map.

What are your thoughts on the above?


return status, nil
}

func (w *Worktree) status(commit plumbing.Hash) (Status, error) {
s := make(Status)
s, err := w.newStatusFromIndex()
if err != nil {
return nil, err
}

left, err := w.diffCommitWithStaging(commit, false)
if err != nil {
Expand Down
19 changes: 19 additions & 0 deletions worktree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,25 @@ func (s *WorktreeSuite) TestStatusEmptyDirty(c *C) {
c.Assert(status, HasLen, 1)
}

func (s *WorktreeSuite) TestStatusUnmodified(c *C) {
fs := memfs.New()
w := &Worktree{
r: s.Repository,
Filesystem: fs,
}

err := w.Checkout(&CheckoutOptions{Force: true})
c.Assert(err, IsNil)

status, err := w.Status()
c.Assert(err, IsNil)
c.Assert(status.IsClean(), Equals, true)
c.Assert(status.IsUntracked("LICENSE"), Equals, false)

c.Assert(status.File("LICENSE").Staging, Equals, Unmodified)
c.Assert(status.File("LICENSE").Worktree, Equals, Unmodified)
}

func (s *WorktreeSuite) TestReset(c *C) {
fs := memfs.New()
w := &Worktree{
Expand Down