Fix migration file names #25735

sorenisanerd · 2023-07-06T16:59:28Z

The file names of the migrations and the comments in
models/migrations/migrations.go disagree on the version number.

For example:

// v259 -> v260
NewMigration("Convert scoped access tokens", v1_20.ConvertScopedAccessTokens),

...but the file containing v1_20.ConvertScopedAccessTokens is called
v1_20/v259.go.

Checking the version table indicates that the comments are correct
and the file names are wrong.

This PR fixes the filenames. It's spread over two commits in an attempt
to make git understand that they're renames. See individual commit
messages for more detail.

The file names of the migrations and the comments in `models/migrations/migrations.go` disagree on the version number. For example: // v259 -> v260 NewMigration("Convert scoped access tokens", v1_20.ConvertScopedAccessTokens), ...but the file containing `v1_20.ConvertScopedAccessTokens` is called `v1_20/v259.go`. Checking the `version` table indicates that the comments are correct and the file names are wrong. git does not explicitly track renames. This causes a problem when we're renaming many files. If a directory had e.g. 3 files in it: v1_50/v500.go v1_50/v501.go v1_50/v502.go and we incremented their filenames like so: v1_50/v500.go -> v1_50/v501.go v1_50/v501.go -> v1_50/v502.go v1_50/v502.go -> v1_50/v503.go git would say that we deleted `v500.go`, changed `v501.go` and `v502.go`, and created a new file, `v503.go`. To work around this, this change is split into two commits. This first one increments all the file names and moves the files to a new directory. git correctly detects these as renames. The next commit will move them all back. This is the command used for this commit (cwd: `models/migrations`): for x in */v*.go do mkdir -p $(dirname $x).new v=$(echo $x | sed -e 's!.*/v$[0-9]*$.*!\1!g') git mv $x $(dirname $x).new/v$(echo $x | sed -e 's@.*/v$[0-9]*$$.*$@'$(($v+1))'\2@g') done

This is the second part of this mass renaming. See previous commit for rationale. This is the command used for this commit (cwd: `models/migrations`): for x in *.new/v*.go do git mv $x $(basename $(dirname $x) .new)/ done

KN4CK3R

Thanks for the individual commits.

Out of curiosity, does Gitea display the changes only as renames or like Github does?

6543 · 2023-07-07T01:34:41Z

the pull diff is hard to read :/

lunny · 2023-07-07T02:05:00Z

Maybe we can rename v259.go -> 260.go so that git can recognize the rename. And next PR we can also rename back if we need that.

wxiaoguang · 2023-07-07T03:51:28Z

The root problem is "how to understand the 'version' and the comment".

For example:

	// v70 -> v71
	NewMigration("add issue_dependencies", v1_6.AddIssueDependencies),

Before:

version means the next migration's starting number
So the number of AddIssueDependencies is 70 (v70.go), and database.version=71 (for next)
The comment means "the current step is v70 and after this step and then the database version becomes 71"

After:

version means the current migration's starting number
So the number of AddIssueDependencies will be 71, and database.version=71 (for current)
The comment for each step can be simplified to "// v71" ...
The comment above (Gitea 1.5.0 ends at v69) seems unmatched, while fortunately, the database version for 1.5.x should be actually 70, so it's only a comment problem).

wxiaoguang · 2023-07-07T04:30:45Z

I also agree with using some tricks like "v259.go -> 260.go" (or v-260.go) is better. (the v prefix seems more readable and easier to maintain)

wolfogre · 2023-07-07T06:17:52Z

My suggestion is v259.go -> v0260.go, so git can recognize the rename and the files order will be always correct (v0001.go ... v9999.go).

If one day we get v9999.go, we should be celebrating, not annoying. 👀

KN4CK3R · 2023-07-07T07:11:55Z

the pull diff is hard to read :/

If you look at the two individual commits you only see renames, so it's fine.

wolfogre · 2023-07-07T07:37:34Z

the pull diff is hard to read :/

If you look at the two individual commits you only see renames, so it's fine.

I don't think it's fine, if the final merged commit cannot recognize the rename, it can be annoying when git blame.

wxiaoguang · 2023-07-07T08:25:30Z

Agree with lunny and wolfogre. When this PR gets "squashed" into main branch, we should make sure there is a clear commit history. Although these two separate commits are clear one by one, but, when they get squashed, the commit history will be a mess.

So, there are some suggestions for "only one clear commit".

lunny: v259.go -> 260.go
wxiaoguang: v259.go -> v-260.go (or use some suffix?) I think a proper prefix really helps about maintainability.
wolfogre: v259.go -> v0260.go

KN4CK3R · 2023-07-07T08:56:28Z

Alternative 260-short_description.go like the text in NewMigration.

GitLab does the same: https://gitlab.com/gitlab-org/gitlab/-/tree/master/db/migrate

6543 · 2023-07-07T11:44:46Z

Hmm or make an exemption and dont squashmerge?

wxiaoguang · 2023-07-10T04:44:48Z

Alternative 260-short_description.go like the text in NewMigration.

GitLab does the same: https://gitlab.com/gitlab-org/gitlab/-/tree/master/db/migrate

I also have asked so, but at that time, in discord discussion, maintainers explained that "Only use the v123.go, to make sure the filename conflicts for different PRs, that's the conflict-based workflow".

I am not sure whether maintainers have new ideas about this topic nowadays.

techknowlogick · 2023-07-10T04:53:46Z

@wxiaoguang I wrote https://github.com/techknowlogick/xormigrate to 1. backport migrations, 2. prevent conflicts from migrates having the same name, and 3. a cleaner developer experience.

wxiaoguang · 2023-08-12T07:03:33Z

Agree with lunny and wolfogre. When this PR gets "squashed" into main branch, we should make sure there is a clear commit history. Although these two separate commits are clear one by one, but, when they get squashed, the commit history will be a mess.

So, there are some suggestions for "only one clear commit".
* lunny: v259.go -> `260.go`

* wxiaoguang: v259.go -> `v-260.go` (or use some suffix?) I think a proper prefix really helps about maintainability.

* wolfogre: v259.go -> `v0260.go`

I think there should be some better solutions than this change, this change's "squash" merge would make the commit unreadable.

dismiss review

sorenisanerd · 2023-08-13T18:16:21Z

Alternatively, we could just get rid of the individual comments in migrations.go? They're not really adding anything but confusion. The order they're listed in is really what matters.

wxiaoguang · 2023-08-15T02:20:57Z

Alternatively, we could just get rid of the individual comments in migrations.go? They're not really adding anything but confusion. The order they're listed in is really what matters.

IMO the comments should be kept, that's the only way (at the moment, by reading code) to know the "version number" of a migration step.

wxiaoguang · 2023-09-07T07:01:01Z

Stale and a lot of conflicts.

Feel free to reopen if some comments (like #25735 (comment)) are addressed. It needs a clear "squahed" result.

wxiaoguang · 2024-10-26T04:17:20Z

-> Refactor the DB migration system slightly #32344

sorenisanerd added 2 commits July 6, 2023 09:33

Fix migration file names - Part 2

fee6800

This is the second part of this mass renaming. See previous commit for rationale. This is the command used for this commit (cwd: `models/migrations`): for x in *.new/v*.go do git mv $x $(basename $(dirname $x) .new)/ done

GiteaBot added the lgtm/need 2 label Jul 6, 2023

pull-request-size bot added the size/XXL label Jul 6, 2023

KN4CK3R approved these changes Jul 6, 2023

View reviewed changes

GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Jul 6, 2023

Merge branch 'main' into migration-rename

c080412

wxiaoguang previously approved these changes Jul 7, 2023

View reviewed changes

GiteaBot added lgtm/done and removed lgtm/need 1 labels Jul 7, 2023

wxiaoguang self-requested a review July 7, 2023 04:26

GiteaBot added lgtm/need 1 and removed lgtm/done labels Jul 7, 2023

wxiaoguang added the issue/needs-feedback label Aug 12, 2023

wxiaoguang removed their request for review August 12, 2023 07:03

GiteaBot removed the lgtm/need 1 label Aug 12, 2023

GiteaBot added the lgtm/done label Aug 12, 2023

GiteaBot added lgtm/need 1 and removed lgtm/done labels Aug 12, 2023

wxiaoguang added the status/blocked label Aug 12, 2023

wxiaoguang closed this Sep 7, 2023

go-gitea locked as resolved and limited conversation to collaborators Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix migration file names #25735

Fix migration file names #25735

sorenisanerd commented Jul 6, 2023

KN4CK3R left a comment

6543 commented Jul 7, 2023

lunny commented Jul 7, 2023 •

edited

Loading

wxiaoguang commented Jul 7, 2023

wxiaoguang commented Jul 7, 2023 •

edited

Loading

wolfogre commented Jul 7, 2023 •

edited

Loading

KN4CK3R commented Jul 7, 2023

wolfogre commented Jul 7, 2023

wxiaoguang commented Jul 7, 2023

KN4CK3R commented Jul 7, 2023 •

edited

Loading

6543 commented Jul 7, 2023

wxiaoguang commented Jul 10, 2023

techknowlogick commented Jul 10, 2023

wxiaoguang commented Aug 12, 2023

sorenisanerd commented Aug 13, 2023

wxiaoguang commented Aug 15, 2023 •

edited

Loading

wxiaoguang commented Sep 7, 2023

wxiaoguang commented Oct 26, 2024

Fix migration file names #25735

Fix migration file names #25735

Conversation

sorenisanerd commented Jul 6, 2023

KN4CK3R left a comment

Choose a reason for hiding this comment

6543 commented Jul 7, 2023

lunny commented Jul 7, 2023 • edited Loading

wxiaoguang commented Jul 7, 2023

wxiaoguang commented Jul 7, 2023 • edited Loading

wolfogre commented Jul 7, 2023 • edited Loading

KN4CK3R commented Jul 7, 2023

wolfogre commented Jul 7, 2023

wxiaoguang commented Jul 7, 2023

KN4CK3R commented Jul 7, 2023 • edited Loading

6543 commented Jul 7, 2023

wxiaoguang commented Jul 10, 2023

techknowlogick commented Jul 10, 2023

wxiaoguang commented Aug 12, 2023

sorenisanerd commented Aug 13, 2023

wxiaoguang commented Aug 15, 2023 • edited Loading

wxiaoguang commented Sep 7, 2023

wxiaoguang commented Oct 26, 2024

lunny commented Jul 7, 2023 •

edited

Loading

wxiaoguang commented Jul 7, 2023 •

edited

Loading

wolfogre commented Jul 7, 2023 •

edited

Loading

KN4CK3R commented Jul 7, 2023 •

edited

Loading

wxiaoguang commented Aug 15, 2023 •

edited

Loading