-
-
Notifications
You must be signed in to change notification settings - Fork 6k
Add reparent
option to Repository CreateFork API
#35430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,7 @@ type ForkRepoOptions struct { | |
Name string | ||
Description string | ||
SingleBranch string | ||
Reparent bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer the previous implementation to have a new API to do the reparent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previous implementation? Or do you mean this should be a new API call, like Repository/CreateNewParent or similar name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do a new API call here, can we still use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think introducing a new API endpoint would be more appropriate. Also, the Reparent field in ForkRepoOptions feels a bit awkward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as we can use the internal Fork implementation already, I'm ok in introducing another API. |
||
} | ||
|
||
// ForkRepository forks a repository | ||
|
@@ -108,8 +109,16 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork | |
if err = createRepositoryInDB(ctx, doer, owner, repo, true); err != nil { | ||
return err | ||
} | ||
if err = repo_model.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil { | ||
return err | ||
|
||
// swap fork_id, if we reparent | ||
if opts.Reparent { | ||
if err = repo_model.ReparentFork(ctx, repo.ID, opts.BaseRepo.ID); err != nil { | ||
return err | ||
} | ||
} else { | ||
if err = repo_model.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
// copy lfs files failure should not be ignored | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fork numbers should be updated as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the source repo, the number of forks remain as-is. Only its new parent is updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fork count of the source repository should be decreased, while the fork count of the new parent repository should be increased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fork count in the source repository is not yet increased here and it never is because it's part of the
else{}
clause in the calling function.The fork of the new parent is set as 1 already.
But I understand how this design is a little confusing. I'll split this into separate API call instead which will make this more straightforward.