refactor: retry file remove/rename when a file is busy and clean up os detection (#38588)

Rename functions "util.Remove" (remove.go) to "util.RemoveWithRetry"
(file_retry.go) and add comments to clarify their behaviors, also add
tests.

Refactor callers: when no concurrent access (cmd cli, migration, app
init, test), use "os.Xxx" directly.

More details are in `modules/util/file_retry.go`

By the way, clean up OS (windows) detection, make FileURLToPath test
always run

modules/git/attribute/main_test.go

@@ -10,7 +10,6 @@ import (
 
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/setting"
-	"gitea.dev/modules/util"
 )
 
 func testRun(m *testing.M) error {
@@ -18,7 +17,7 @@ func testRun(m *testing.M) error {
 	if err != nil {
 		return fmt.Errorf("unable to create temp dir: %w", err)
 	}
-	defer util.RemoveAll(gitHomePath)
+	defer os.RemoveAll(gitHomePath)
 	setting.Git.HomePath = gitHomePath
 
 	if err = git.InitFull(); err != nil {

modules/git/catfile_batch_reader.go

@@ -25,12 +25,16 @@ type catFileBatchCommunicator struct {
 	reqWriter   io.Writer
 	respReader  *bufio.Reader
 	debugGitCmd *gitcmd.Command
+	closed      chan struct{}
 }
 
 func (b *catFileBatchCommunicator) Close(err ...error) {
 	if fn := b.closeFunc.Swap(nil); fn != nil {
 		(*fn)(util.OptionalArg(err))
 	}
+	// make sure the git process has fully exited before we return from Close()
+	// otherwise, the opened files will block the directory renaming (rename a repo) on Windows
+	<-b.closed
 }
 
 // newCatFileBatch opens git cat-file --batch/--batch-check/--batch-command command and prepares the stdin/stdout pipes for communication.
@@ -41,6 +45,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git
 		debugGitCmd: cmdCatFile,
 		reqWriter:   stdinWriter,
 		respReader:  bufio.NewReaderSize(stdoutReader, 32*1024), // use a buffered reader for rich operations
+		closed:      make(chan struct{}),
 	}
 	ret.closeFunc.Store(new(func(err error) {
 		ctxCancel(err)
@@ -52,6 +57,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git
 		log.Error("Unable to start git command %v: %v", cmdCatFile.LogString(), err)
 		// ideally here it should return the error, but it would require refactoring all callers
 		// so just return a dummy communicator that does nothing, almost the same behavior as before, not bad
+		close(ret.closed)
 		ret.Close(err)
 		return ret
 	}
@@ -61,6 +67,7 @@ func newCatFileBatch(ctx context.Context, repo RepositoryFacade, cmdCatFile *git
 		if err != nil && !errors.Is(err, context.Canceled) {
 			log.Error("cat-file --batch command failed in repo %s, error: %v", repo.LogString(), err)
 		}
+		close(ret.closed)
 		ret.Close(err)
 	}()
 

modules/git/hook.go

@@ -72,16 +72,11 @@ func (h *Hook) Name() string {
 // Update updates hook settings.
 func (h *Hook) Update() error {
 	if len(strings.TrimSpace(h.Content)) == 0 {
-		exist, err := util.IsExist(h.path)
-		if err != nil {
+		// empty content means to remove the file
+		err := util.RemoveWithRetry(h.path)
+		if err != nil && !os.IsNotExist(err) {
 			return err
 		}
-		if exist {
-			err := util.Remove(h.path)
-			if err != nil {
-				return err
-			}
-		}
 		h.IsActive = false
 		return nil
 	}

modules/git/hooks.go

@@ -124,7 +124,7 @@ func createDelegateHooks(hookDir string) (err error) {
 		}
 
 		// WARNING: This will override all old server-side hooks
-		if err = util.Remove(oldHookPath); err != nil && !os.IsNotExist(err) {
+		if err = util.RemoveWithRetry(oldHookPath); err != nil && !os.IsNotExist(err) {
 			return fmt.Errorf("unable to pre-remove old hook file '%s' prior to rewriting: %w ", oldHookPath, err)
 		}
 		if err = os.WriteFile(oldHookPath, []byte(hookTpls[i]), 0o777); err != nil {
@@ -135,7 +135,7 @@ func createDelegateHooks(hookDir string) (err error) {
 			return fmt.Errorf("Unable to set %s executable. Error %w", oldHookPath, err)
 		}
 
-		if err = util.Remove(newHookPath); err != nil && !os.IsNotExist(err) {
+		if err = util.RemoveWithRetry(newHookPath); err != nil && !os.IsNotExist(err) {
 			return fmt.Errorf("unable to pre-remove new hook file '%s' prior to rewriting: %w", newHookPath, err)
 		}
 		if err = os.WriteFile(newHookPath, []byte(giteaHookTpls[i]), 0o777); err != nil {

modules/git/localfs.go

@@ -23,7 +23,7 @@ func IsRepositoryExist(ctx context.Context, repo RepositoryFacade) (bool, error)
 // DeleteRepository deletes the repository directory from the disk, it will return
 // nil if the repository does not exist.
 func DeleteRepository(ctx context.Context, repo RepositoryFacade) error {
-	return util.RemoveAll(gitrepo.RepoLocalPath(repo))
+	return util.RemoveAllWithRetry(gitrepo.RepoLocalPath(repo))
 }
 
 // RenameRepository renames a repository's name on disk
@@ -33,7 +33,7 @@ func RenameRepository(ctx context.Context, repo, newRepo RepositoryFacade) error
 		return fmt.Errorf("Failed to create dir %s: %w", filepath.Dir(dstDir), err)
 	}
 
-	if err := util.Rename(gitrepo.RepoLocalPath(repo), dstDir); err != nil {
+	if err := util.RenameWithRetry(gitrepo.RepoLocalPath(repo), dstDir); err != nil {
 		return fmt.Errorf("rename repository directory: %w", err)
 	}
 	return nil
@@ -59,7 +59,7 @@ func IsRepoDirExist(ctx context.Context, repo RepositoryFacade, relativeDirPath
 
 func RemoveRepoFileOrDir(ctx context.Context, repo RepositoryFacade, relativeFileOrDirPath string) error {
 	absoluteFilePath := filepath.Join(gitrepo.RepoLocalPath(repo), relativeFileOrDirPath)
-	return util.Remove(absoluteFilePath)
+	return util.RemoveWithRetry(absoluteFilePath)
 }
 
 func CreateRepoFile(ctx context.Context, repo RepositoryFacade, relativeFilePath string) (io.WriteCloser, error) {