new methods Worktree.[AddGlob|RemoveBlob] and recursive Worktree.[Add|Remove] #739

mcuadros · 2018-02-01T18:18:30Z

Fixes: #456

Signed-off-by: Máximo Cuadros <[email protected]>

orirawlings · 2018-02-07T15:15:36Z

worktree_status.go

+// AddDirectory adds the files contents of a directory and all his
+// sub-directories recursively in the worktree to the index. If any of the
+// file is already staged in the index no error is returned.
+func (w *Worktree) AddDirectory(path string) error {


This API seems a little clumsy to me. Is there a reason we can't simply detect and support directory paths as part of the current Add function? A path string is already sufficient for a user to describe either directories or files.

Moreover, if we are going to provide separate functions for directories on Add we should also include separate functions MoveDirectory and RemoveDirectory for consistency.

In any case, I think we should simply extend the existing Add, Move, and Remove to support directory paths.

what should I return when I Add a repository? Because the signature returns a Hash.

It seems like Add returns the hash of the blob object that was added to the repository. If one is adding a directory, it might make sense to return the hash of the corresponding tree object.

Add doesn't create any tree object.

Ah yes. I see that. I guess this works then.

orirawlings · 2018-02-07T15:19:26Z

worktree_status.go

+// AddGlob given a glob pattern adds all the matching files content and all his
+// sub-directories recursively in the worktree to the index. If any of the
+// file is already staged in the index no error is returned.
+func (w *Worktree) AddGlob(pattern string) error {


If we are going to support AddGlob we should also add a RemoveGlob for consistency in the API.

Nice point.

mcuadros · 2018-02-18T23:35:25Z

@orirawlings PTAL

orirawlings

This is looking really good. :)

I just had a few minor suggestions and questions.

orirawlings · 2018-02-20T15:45:22Z

plumbing/format/index/index.go

@@ -51,8 +53,20 @@ type Index struct {
 	ResolveUndo *ResolveUndo
 }

+// Add creates a new Entry and returns it. The caller should check that not


The caller should first check that another entry with the same path does not exist.

orirawlings · 2018-02-20T15:47:29Z

worktree_status.go

+	// the worktree.
+	ErrDestinationExists = errors.New("destination exists")
+	// ErrGlobNoMatches in an AddGlob if the glob pattern does not match any
+	// file in the worktree.ErrNotDirectory


Extra ErrNotDirectory at end of this comment?

orirawlings · 2018-02-20T15:48:27Z

worktree_status.go

+	ErrDestinationExists = errors.New("destination exists")
+	// ErrGlobNoMatches in an AddGlob if the glob pattern does not match any
+	// file in the worktree.ErrNotDirectory
+	ErrGlobNoMatches = errors.New("glob pattern did not match any files")


match any paths

?

And maybe change the comment to refer to path instead of file as well?

What match is files, so I thing is ok with files

orirawlings · 2018-02-20T15:57:38Z

worktree_status.go

+	return
+}
+
+// AddGlob given a glob pattern adds all the matching files content and all his


Might be good to avoid the masculine pronoun, "his".

AddGlob adds all paths, matching pattern, to the index. If pattern matches a directory path, all directory contents are added to the index recursively. No error is returned if all matching paths are already staged in index.

orirawlings · 2018-02-20T16:03:41Z

worktree_status.go

 		}
-		return h, err
+
+		return
 	}

 	if s.File(path).Worktree == Unmodified {


Would it be an optimization to move this check before the w.copyFileToStorage(path) call?

orirawlings · 2018-02-20T16:11:58Z

worktree_status.go

 	if err != nil {
 		return plumbing.ZeroHash, err
 	}

+	if err := w.deleteFromFilesystem(path); err != nil {


Maybe just:

return hash, w.deleteFromFilesystem(path)

orirawlings · 2018-02-20T16:14:18Z

worktree_status.go

@@ -409,6 +585,43 @@ func (w *Worktree) deleteFromFilesystem(path string) error {
 	return err
 }

+// RemoveGlob given a glob pattern removes all the matching files content and


We should probably avoid the masculine pronoun, "his", here too:

RemoveGlob removes all paths, matching pattern, from the index. If pattern matches a directory path, all directory contents are removed from the index recursively.

orirawlings · 2018-02-20T16:19:53Z

worktree_status.go

+		return err
+	}
+
+	if len(files) == 0 {


I'm curious here how we might use this method for a paths that might still exist in the repository or index, but might have already been removed from the working directory. Will this check cause this method to return an error without attempting to remove them from the index?

Example:

$ git init $ touch a; git add a; git commit -m first [master (root-commit) 5d36496] first 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 a $ rm a; git status On branch master Changes not staged for commit: (use "git add/rm <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) deleted: a no changes added to commit (use "git add" and/or "git commit -a") $ git rm -- a*; git status rm 'a' On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) deleted: a

@mcuadros this seems like a bug to me. Can you confirm?

In git add, you can modify entries in the index even if the glob matches no paths in the current working directory. But this check looks like it would return an error before trying to update the index if the glob has no matches in the working directory.

$ git init $ touch a; git add a $ rm a; ls a* $ git add -- a*; echo $? 0

I think this is a very complex behavior, the goal of this PR was speed up multiple Add calls.

orirawlings · 2018-02-20T16:23:02Z

worktree_status.go

+
+	return w.r.Storer.SetIndex(idx)
+}
+
 // Move moves or rename a file in the worktree and the index, directories are
 // not supported.
 func (w *Worktree) Move(from, to string) (plumbing.Hash, error) {


Are we planning to wait for now on adding support for directories to this method?

Signed-off-by: Máximo Cuadros <[email protected]>

…|Remove] Signed-off-by: Máximo Cuadros <[email protected]>

mcuadros · 2018-02-25T23:27:47Z

@orirawlings PTAL

orirawlings

Sorry, I didn't have a look sooner.

Looks good. I just have one open question about using AddGlob to remove files from the index. It looks to me like the current code doesn't support that, but git add -- <glob> does.

orirawlings · 2018-02-28T15:42:19Z

worktree_status.go

+		return err
+	}
+
+	if len(files) == 0 {


@mcuadros this seems like a bug to me. Can you confirm?

In git add, you can modify entries in the index even if the glob matches no paths in the current working directory. But this check looks like it would return an error before trying to update the index if the glob has no matches in the working directory.

$ git init $ touch a; git add a $ rm a; ls a* $ git add -- a*; echo $? 0

orirawlings · 2018-02-28T15:50:30Z

worktree_status.go

+	}
+
+	var saveIndex bool
+	for _, file := range files {


This also looks like we aren't trying to match patterns on things in the index, only paths that exist in the working directory. This is slightly different behavior than git add -- <glob> in cases where index entries should be removed (i.e. git add supports both adding AND removing entries from the index).

Add, only adds files from the woktree to the index. The function Add should not do exactly the behavior of git add. This is a programmatically interface, so I rather keep it simple.

I think that makes sense. I'm just anticipating that this discrepancy will cause confusion for users and drive more issues being opened.

inge4pres · 2018-09-19T07:30:49Z

Hi 😄
can't understand if this is adding the support for recursive deletion of not empty folders, I am trying to do it with
worktree.Filesystem.Remove(path) where path is a folder containing some files and I'm getting error remove /tmp/path: directory not empty (using the clone with OS storage)

I looked a bit into billy's code and there is a RemoveAll function mimicking the standard lib's os.RemoveAll but it looks like in worktree.go it's only used in the rmFileAndDirIfEmpty function...

Any ideas?
Thanks

new methods Worktree.[AddGlob|AddDirectory]

66f08b8

Signed-off-by: Máximo Cuadros <[email protected]>

mcuadros requested a review from ajnavarro February 1, 2018 18:18

orirawlings suggested changes Feb 7, 2018

View reviewed changes

mcuadros changed the title ~~new methods Worktree.[AddGlob|AddDirectory]~~ new methods Worktree.[AddGlob|RemoveBlob] and recurive Add and Remove Feb 18, 2018

mcuadros changed the title ~~new methods Worktree.[AddGlob|RemoveBlob] and recurive Add and Remove~~ new methods Worktree.[AddGlob|RemoveBlob] and recursive Worktree.[Add|Remove] Feb 18, 2018

orirawlings reviewed Feb 20, 2018

View reviewed changes

mcuadros added 2 commits February 26, 2018 00:26

plumbing: format index, Index.Add and Index.Glob methods

9fb58fc

Signed-off-by: Máximo Cuadros <[email protected]>

new methods Worktree.[AddGlob|RemoveBlob] and recursive Worktree.[Add…

6d23b50

…|Remove] Signed-off-by: Máximo Cuadros <[email protected]>

orirawlings reviewed Feb 28, 2018

View reviewed changes

orirawlings approved these changes Feb 28, 2018

View reviewed changes

mcuadros merged commit 4397264 into src-d:master Feb 28, 2018

inge4pres mentioned this pull request Sep 19, 2018

How to remove recursively a directory? #961

Closed

new methods Worktree.[AddGlob|RemoveBlob] and recursive Worktree.[Add|Remove] #739

new methods Worktree.[AddGlob|RemoveBlob] and recursive Worktree.[Add|Remove] #739

Uh oh!

Conversation

mcuadros commented Feb 1, 2018

Uh oh!

orirawlings Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcuadros commented Feb 18, 2018

Uh oh!

orirawlings left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcuadros Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcuadros commented Feb 25, 2018

Uh oh!

orirawlings left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inge4pres commented Sep 19, 2018

Uh oh!

Uh oh!

orirawlings Feb 7, 2018 •

edited

Loading

mcuadros Feb 25, 2018 •

edited

Loading