Use case-insensitive matching for Git error "Not a valid object name"

[mdferdousalam]

Fixes #36727

Git is lowercasing the fatal: Not a valid object name error message
to follow its CodingGuidelines. This change makes the string matching
case-insensitive so it works with both the current and future Git versions.

Changes:

• Use strings.ToLower() before checking for the error substring

See: https://lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com

[wxiaoguang]

Thank you very much. I will try to extract the logic into a shared function to improve maintainability.

[silverwind]

I'm not sure the refactor is warrented. Previous code was easy to read with two strings.Contains calls, now the reader has to jump to another file. I think if it's refactored, it should be a errorMessageContainsAny(err, []string).

[silverwind]

I investigated for more git errors that are matched case-sensitively in the code base and found these:

• "fatal: Needed a single revision" in modules/git/repo_commit_gogit.go:61
• "Not possible to fast-forward, aborting" in services/pull/merge.go:526
• "Unable to create" in services/mirror/mirror_pull.go:101
• "Authentication failed" in services/task/migrate.go:144, routers/api/v1/repo/migrate.go:248, routers/web/repo/migrate.go:112
• "Bad credentials" in routers/api/v1/repo/migrate.go:249, routers/web/repo/migrate.go:113
• "could not read Username" in services/task/migrate.go:145, routers/web/repo/migrate.go:114

Maybe we should fix them all here, or in another PR.

[wxiaoguang]

I'm not sure the refactor is warrented. Previous code was easy to read with two strings.Contains calls, now the reader has to jump to another file. I think if it's refactored, it should be a errorMessageContainsAny(err, []string).

No

I investigated for more git errors that are matched case-sensitively in the code base and found these:

They are different cases.

---

Using magic string is fragile

[silverwind]

They are different cases.

Do you agree they need fixing?

Using magic string is fragile

Could be made constants

[lunny]

lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com

But upstream doesn't convert them to lower case lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com

[silverwind]

lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com

But upstream doesn't convert them to lower case lore.kernel.org/git/pull.2052.git.1771836302101.gitgitgadget@gmail.com

Not yet, but I assume git authors will want to eventually unify all their messages to lowercase and we could be pre-prepared for when it happens.

[wxiaoguang]

They are different cases.

Do you agree they need fixing?

No

Using magic string is fragile

Could be made constants

No

Not necessary at the moment, can be done later when there are enough good use cases.

I have designed the git error system and made it flexible enough. For example:

---

To introduce more "constants", need to investigate the related code and make a general design, not in this PR's scope and I don't want to do it at the moment.

[wxiaoguang]

To introduce more "constants", need to investigate the related code and make a general design, not in this PR's scope and I don't want to do it at the moment.

By the way, the design can be like this, just FYI:

gitcmd.StderrEqual(err, ...)
gitcmd.StderrEquaFold(err, ...)
gitcmd.StderrHasPrefix(err, ...)
gitcmd.StderrHasPrefixFold(err, ...)
gitcmd.StderrContainsAny(err, ...)
gitcmd.StderrContainsAnyFold(err, ...)

Or always do "fold" comparing, only keep StderrEqual, StderrHasPrefix and StderrContainsAny.

Another idea is to introduce enough functions for different error types, and sometimes we need to parse the git's stderr.

---

However, I don't think these ideas are mature enough, haven't spend time on it.

[silverwind]

Yeah anything like that is fine, I just don't want gitea to break when git changes casing in more error messages in the future. Matching error message by string is already a bad API, we should make it liberally accept any casing.

[wxiaoguang]

Yeah anything like that is fine, I just don't want gitea to break when git changes casing in more error messages in the future. Matching error message by string is already a bad API, we should make it liberally accept any casing.

Yes, that's why I made many changes to the "git" packages. https://github.com/go-gitea/gitea/pulls?q=is%3Apr+git+is%3Aclosed+author%3Awxiaoguang There are also some comments describing the future direction.

Till now, the situation is much better than before, but there are still many legacy problems like this:

func (r *runStdError) Error() string {
	// FIXME: GIT-CMD-STDERR: it is a bad design, the stderr should not be put in the error message
	// But a lot of code only checks `strings.Contains(err.Error(), "git error")`
	....

[silverwind]

I wonder if some of these cases could be changed to determine outcome by exit code instead of message.

[wxiaoguang]

Answer is in code