Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file #35151
Conversation
…lve possible conflict when updating repository git config file
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
|
No, it doesn't fix #32018 |
GiteaBot
added
lgtm/blocked
In cases where |
|
So it is an improvement but doesn't "Fix #32018", right? But does it really need the "global lock"? Is it possible that git already uses "*.lock" files by |
The Git command-line interface is primarily designed for human interaction. When a git config write operation is initiated and a config.lock file is created, any subsequent git config write operation will immediately fail instead of waiting for the first one to complete. While I couldn’t find official documentation confirming this behavior, the implementation can be found in the Git source code here: Based on this, it appears that Git does not implement a locking mechanism that causes subsequent operations to wait—it simply fails fast. I believe this change partially addresses #32018, as there are three possible causes for the issue:
|
|
Git already does a file-based "global lock"
Update: git passes timeout=0, so it only locks "once" |
GiteaBot
added
lgtm/need 2
…a into lunny/fix_git_config_conflict
|
last call @go-gitea/technical-oversight-committee |
…a into lunny/fix_git_config_conflict
…a into lunny/fix_git_config_conflict
GiteaBot
added
lgtm/done
| return giturl.ParseGitURL(addr) | ||
| } | ||
|
|
||
| func SetRemoteURL(ctx context.Context, repo Repository, remoteName, remoteURL string) error { |
There was a problem hiding this comment.
It should be named as GitRemoteSetURL to match other functions and the command itself git remote set-url
| } | ||
|
|
||
| // GitRemoteUpdatePrune updates the remote branches and prunes the ones that no longer exist in the remote repository. | ||
| // No lock is needed because the remote remoteName will be checked before invoking this function. |
There was a problem hiding this comment.
Where is the "remoteName will be checked"?
| } | ||
|
|
||
| // GitRemotePrune prunes the remote branches that no longer exist in the remote repository. | ||
| // No lock is needed because the remote remoteName will be checked before invoking this function. |
There was a problem hiding this comment.
Where is the "remoteName will be checked"?
| return "", err | ||
| } | ||
| if len(result) > 0 { | ||
| result = result[:len(result)-1] // remove trailing newline |
There was a problem hiding this comment.
Is it also true for Windows? \r\n or only \n?
| _, err := git.GetRemoteAddress(ctx, m.Repo.WikiPath(), m.RemoteName) | ||
| if err == nil { | ||
| u, err := gitrepo.GitRemoteGetURL(ctx, m.Repo.WikiStorageRepo(), m.RemoteName) | ||
| if err == nil && u != nil { |
There was a problem hiding this comment.
if err is nil, then u isn't nil
Signed-off-by: wxiaoguang <[email protected]>
| } | ||
| defer func() { | ||
| if err := repo.RemoveRemote(tmpRemote); err != nil { | ||
| log.Error("getMergeBase: RemoveRemote: %v", err) | ||
| if err := gitrepo.GitRemoteRemove(ctx, repo, tmpRemote); err != nil { |
There was a problem hiding this comment.
I don't think it can use ctx here. In this defer, the ctx might have been canceled.
| return nil, fmt.Errorf("GitRemoteAdd: %w", err) | ||
| } | ||
| defer func() { | ||
| if err := gitrepo.GitRemoteRemove(ctx, headRepo, tmpRemote); err != nil { |
There was a problem hiding this comment.
Same as above, it can't use ctx
Partially fix #32018
git configandgit remotewrite operations create a temporary file namedconfig.lock. Since these operations are not atomic, they must not be run in parallel. If two requests attempt to modify the same repository concurrently—such as during a compare operation—one may fail due to the presence of an existingconfig.lockfile.In cases where
config.lockis left behind due to an unexpected program exit, a global lock mechanism could allow us to safely remove the stale lock file when a related error is detected. While this behavior is not yet implemented in this PR, it is planned for a future enhancement.