Skip to content
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

Make git push options accept short name #32245

Merged
merged 4 commits into from
Oct 12, 2024
Merged

Conversation

wxiaoguang
Copy link
Contributor

Just like what most CLI parsers do: --opt means opt=true

Then users could use -o force-push as -o force-push=true

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 12, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Oct 12, 2024
@wxiaoguang wxiaoguang force-pushed the fix-git-opt branch 2 times, most recently from 8268649 to 3f0d1de Compare October 12, 2024 04:06
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 12, 2024
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Oct 12, 2024
@@ -208,7 +208,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
return
}

cols := make([]string, 0, len(opts.GitPushOptions))
cols := make([]string, 0, 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using len of push options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why using len of push options?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why using len of push options?

I think it's better set a length when we know the max length of a slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there could be far more push options than it really needs here.

At most, here it only needs 2, but there could be 4 or 5 or even more push options. They are not related.

Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. by the way, suggest add this test function also.

func TestAgitPullPush(t *testing.T) {
	onGiteaRun(t, func(t *testing.T, u *url.URL) {
		baseAPITestContext := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)

		u.Path = baseAPITestContext.GitPath()
		u.User = url.UserPassword("user2", userPassword)

		dstPath := t.TempDir()
		doGitClone(dstPath, u)(t)

		gitRepo, err := git.OpenRepository(context.Background(), dstPath)
		assert.NoError(t, err)
		defer gitRepo.Close()

		doGitCreateBranch(dstPath, "test-topic")

		// commit 1
		_, err = generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-")
		assert.NoError(t, err)

		// push to create a agit pull request
		err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-topic").Run(&git.RunOpts{Dir: dstPath})
		assert.NoError(t, err)

		// check pull request exist
		unittest.AssertExistsAndLoadBean(t, &issues.PullRequest{BaseRepoID: 1, Flow: issues.PullRequestFlowAGit, HeadBranch: "user2/test-topic"})

		// commit 2
		_, err = generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-2-")
		assert.NoError(t, err)

		// push 2
		err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-topic").Run(&git.RunOpts{Dir: dstPath})
		assert.NoError(t, err)

		// reset to first commit
		err = git.NewCommand(git.DefaultContext, "reset", "--hard", "HEAD~1").Run(&git.RunOpts{Dir: dstPath})
		assert.NoError(t, err)

		// test force push without confirm
		stderr := strings.Builder{}
		err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-topic").Run(&git.RunOpts{Dir: dstPath, Stderr: &stderr})
		assert.Error(t, err)
		assert.Contains(t, stderr.String(), "[remote rejected] HEAD -> refs/for/master/test-topic (request `force-push` push option)")

		// test force push with confirm
		err = git.NewCommand(git.DefaultContext, "push", "origin", "HEAD:refs/for/master/test-topic", "-o", "force-push").Run(&git.RunOpts{Dir: dstPath, Stderr: &stderr})
		assert.NoError(t, err)
	})
}

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 12, 2024
@wxiaoguang
Copy link
Contributor Author

LGTM. by the way, suggest add this test function also.

Great, thank you for the awesome test.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 12, 2024
@lunny lunny enabled auto-merge (squash) October 12, 2024 05:01
@lunny lunny merged commit afa8dd4 into go-gitea:main Oct 12, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 12, 2024
@wxiaoguang wxiaoguang deleted the fix-git-opt branch October 12, 2024 05:51
@lunny
Copy link
Member

lunny commented Oct 13, 2024

docs update https://gitea.com/gitea/docs/pulls/80

zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 16, 2024
* giteaofficial/main:
  Make `owner/repo/pulls` handlers use "PR reader" permission (go-gitea#32254)
  make `show stats` work when only one file changed (go-gitea#32244)
  Update scheduled tasks even if changes are pushed by "ActionsUser" (go-gitea#32246)
  Support migrating GitHub/GitLab PR draft status (go-gitea#32242)
  Only rename a user when they should receive a different name (go-gitea#32247)
  Fix dropdown content overflow (go-gitea#31610)
  Make git push options accept short name (go-gitea#32245)
  Allow code search by filename (go-gitea#32210)
  Allow maintainers to view and edit files of private repos when "Allow maintainers to edit" is enabled (go-gitea#32215)
  Use per package global lock for container uploads instead of memory lock (go-gitea#31860)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants