add `tea pulls [checkout | clean]` commands (#93 #97 #107) #105

Merged
lunny merged 20 commits from noerw/tea:issue-97/pulls-clean into master 2 years ago
noerw commented 3 years ago
Collaborator

fixes #93, #97, #107

fixes #93, #97, #107
lunny reviewed 3 years ago
Dismissed
cmd/pulls.go Outdated
if err != nil {
return err
}
if pr.State == gitea.StateOpen {
Owner

User may still want to clean it, but we can give a confirmation.

User may still want to clean it, but we can give a confirmation.
Poster
Collaborator

But we can't safely remove the remote branch in this case, right?
Effectively tea wouldn't do anything different than git branch -D <featurebranch>, so I don't see the point.
We could guide the user to call tea pulls close 1 first if he really wants to, thats more expressive than a --force flag

But we can't safely remove the remote branch in this case, right? Effectively tea wouldn't do anything different than `git branch -D <featurebranch>`, so I don't see the point. We could guide the user to call `tea pulls close 1` first if he really wants to, thats more expressive than a `--force` flag
lunny reviewed 3 years ago
Dismissed
package git
Owner

copyright head.

copyright head.
lunny added the kind/feature label 3 years ago
lunny added this to the v0.3.0 milestone 3 years ago
Owner

I encountered an panic when I input tea pr clean 7 on xorm/reverse#7.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x14becf0]

goroutine 1 [running]:
code.gitea.io/tea/modules/git.TeaRepo.TeaFindBranch.func1(0xc0000204c0, 0xc0000204c0, 0x0)
	/gitea/tea/modules/git/branch.go:96 +0x190
gopkg.in/src-d/go-git.v4/plumbing/storer.(*referenceFilteredIter).ForEach(0xc0001aeec0, 0xc00019e810, 0x0, 0x0)
	/gitea/tea/vendor/gopkg.in/src-d/go-git.v4/plumbing/storer/reference.go:82 +0xb8
code.gitea.io/tea/modules/git.TeaRepo.TeaFindBranch(0xc00019e4b0, 0xc0001aa450, 0x28, 0xc0001aa780, 0x22, 0x7, 0xc000190140, 0x0)
	/gitea/tea/modules/git/branch.go:88 +0x1b7
code.gitea.io/tea/cmd.runPullsClean(0xc0001808c0, 0x5, 0x8)
	/gitea/tea/cmd/pulls.go:188 +0x24f
github.com/urfave/cli/v2.(*Command).Run(0x1b9f080, 0xc000180600, 0x0, 0x0)
	/gitea/tea/vendor/github.com/urfave/cli/v2/command.go:161 +0x4b9
github.com/urfave/cli/v2.(*App).RunAsSubcommand(0xc000001980, 0xc0001802c0, 0x0, 0x0)
	/gitea/tea/vendor/github.com/urfave/cli/v2/app.go:433 +0xa16
github.com/urfave/cli/v2.(*Command).startApp(0x1b9ee40, 0xc0001802c0, 0xc000140ae0, 0x2)
	/gitea/tea/vendor/github.com/urfave/cli/v2/command.go:275 +0x67f
github.com/urfave/cli/v2.(*Command).Run(0x1b9ee40, 0xc0001802c0, 0x0, 0x0)
	/tea/vendor/github.com/urfave/cli/v2/command.go:91 +0xa25
github.com/urfave/cli/v2.(*App).RunContext(0xc000001800, 0x17a25c0, 0xc000028108, 0xc000020080, 0x4, 0x4, 0x0, 0x0)
	/tea/vendor/github.com/urfave/cli/v2/app.go:302 +0x790
github.com/urfave/cli/v2.(*App).Run(...)
	/tea/vendor/github.com/urfave/cli/v2/app.go:211
main.main()
	/tea/main.go:47 +0x1e8
I encountered an panic when I input `tea pr clean 7` on https://gitea.com/xorm/reverse/pulls/7. ```log panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x14becf0] goroutine 1 [running]: code.gitea.io/tea/modules/git.TeaRepo.TeaFindBranch.func1(0xc0000204c0, 0xc0000204c0, 0x0) /gitea/tea/modules/git/branch.go:96 +0x190 gopkg.in/src-d/go-git.v4/plumbing/storer.(*referenceFilteredIter).ForEach(0xc0001aeec0, 0xc00019e810, 0x0, 0x0) /gitea/tea/vendor/gopkg.in/src-d/go-git.v4/plumbing/storer/reference.go:82 +0xb8 code.gitea.io/tea/modules/git.TeaRepo.TeaFindBranch(0xc00019e4b0, 0xc0001aa450, 0x28, 0xc0001aa780, 0x22, 0x7, 0xc000190140, 0x0) /gitea/tea/modules/git/branch.go:88 +0x1b7 code.gitea.io/tea/cmd.runPullsClean(0xc0001808c0, 0x5, 0x8) /gitea/tea/cmd/pulls.go:188 +0x24f github.com/urfave/cli/v2.(*Command).Run(0x1b9f080, 0xc000180600, 0x0, 0x0) /gitea/tea/vendor/github.com/urfave/cli/v2/command.go:161 +0x4b9 github.com/urfave/cli/v2.(*App).RunAsSubcommand(0xc000001980, 0xc0001802c0, 0x0, 0x0) /gitea/tea/vendor/github.com/urfave/cli/v2/app.go:433 +0xa16 github.com/urfave/cli/v2.(*Command).startApp(0x1b9ee40, 0xc0001802c0, 0xc000140ae0, 0x2) /gitea/tea/vendor/github.com/urfave/cli/v2/command.go:275 +0x67f github.com/urfave/cli/v2.(*Command).Run(0x1b9ee40, 0xc0001802c0, 0x0, 0x0) /tea/vendor/github.com/urfave/cli/v2/command.go:91 +0xa25 github.com/urfave/cli/v2.(*App).RunContext(0xc000001800, 0x17a25c0, 0xc000028108, 0xc000020080, 0x4, 0x4, 0x0, 0x0) /tea/vendor/github.com/urfave/cli/v2/app.go:302 +0x790 github.com/urfave/cli/v2.(*App).Run(...) /tea/vendor/github.com/urfave/cli/v2/app.go:211 main.main() /tea/main.go:47 +0x1e8 ```
noerw changed title from add `tea pulls clean` command (#97) to add `tea pulls [checkout | clean]` commands (#93 #97 #107) 3 years ago
Poster
Collaborator

@lunny There were flaws in the way I looked up branches, should be fixed now among other problems.

@lunny There were flaws in the way I looked up branches, should be fixed now among other problems.
6543 requested changes 3 years ago
Dismissed
return err
}
idx, err := argToIndex(index)
Collaborator

no err check

no err check
Poster
Collaborator

thanks, resolved

thanks, resolved
6543 reviewed 3 years ago
Dismissed
)
// CmdPulls represents to login a gitea server.
// CmdPulls is the main command to operate on PRs
Collaborator

good catch!

good catch!
6543 reviewed 3 years ago
Dismissed
}
// verify related remote is in local repo, otherwise add it
newRemoteName := fmt.Sprintf("pulls/%v", pr.Head.Repository.Owner.UserName)
Collaborator

future impruvement: define prefix via config ... (for examle i sue user/)

but this can be added after this pull :)

future impruvement: define prefix via config ... (for examle i sue `user/`) but this can be added after this pull :)
6543 reviewed 3 years ago
Dismissed
return fmt.Errorf("PR is still open, won't delete branches")
}
// check if the feature branch is ours:
Collaborator

can you move L205-207 too current 213 ...?

can you move L205-207 too current 213 ...?
6543 approved these changes 3 years ago
Dismissed
6543 left a comment
Collaborator

a few questions ... but looks good

a few questions ... but looks good
localBranchRefName := git_plumbing.NewBranchReferenceName(localBranchName)
err := r.CreateBranch(&git_config.Branch{
Name: localBranchName,
Merge: git_plumbing.NewBranchReferenceName(remoteBranchName), // FIXME: should be remoteBranchName
Collaborator

Whats the meaning of the FIXME?

Whats the meaning of the FIXME?
Owner

Panic is gone, but it reported no authentication but in fact when I input tea issues, there are content listed.

Checking out 'master' to delete local branch 'lunny/reverse_reader'
Deleting local branch lunny/reverse_reader and remote branch lunny/reverse_reader
2020/04/01 16:41:44 Failed to run app with [tea pr clean 7]: authentication required
Panic is gone, but it reported no authentication but in fact when I input `tea issues`, there are content listed. ``` Checking out 'master' to delete local branch 'lunny/reverse_reader' Deleting local branch lunny/reverse_reader and remote branch lunny/reverse_reader 2020/04/01 16:41:44 Failed to run app with [tea pr clean 7]: authentication required ```
Poster
Collaborator

@lunny Interesting. I guess you use a https remote? For SSH it worked fine.
I'm not sure how one would safely provide interactive authentication for https.

When developing, I noted that in pushing/pulling you can provide an AuthMethod, for which I found no further documentation though, so I'd need help here

@lunny Interesting. I guess you use a `https` remote? For SSH it worked fine. I'm not sure how one would safely provide interactive authentication for https. When developing, I noted that in pushing/pulling you can provide an [`AuthMethod`](https://godoc.org/gopkg.in/src-d/go-git.v4/plumbing/transport#AuthMethod), for which I found no further documentation though, so I'd need help here
Owner

We should care that the head branch maybe in the forked repsitory and a non-origin remote.

We should care that the head branch maybe in the forked repsitory and a non-origin remote.
Poster
Collaborator

You mean that we should not clean on remotes that are not origin, as the user might have no write permission? Whitelisting only origin is too strict (i use origin as upstream and noerw for my fork).
I guess there is is no other safe way than trying if the user may push there, and notifying that the remote branch deletion failed

You mean that we should not clean on remotes that are not `origin`, as the user might have no write permission? Whitelisting only `origin` is too strict (i use origin as upstream and `noerw` for my fork). I guess there is is no other safe way than trying if the user may push there, and notifying that the remote branch deletion failed
Collaborator
Fetching PR 333 (head https://gitea.com/gitnex/GitNex.git:304-popup-menus-to-bottomsheets) from remote 'main'
2020/04/02 00:24:56 Failed to run app with [tea pr checkout 333]: error creating SSH agent: "SSH agent requested but SSH_AUTH_SOCK not-specified"
```sh Fetching PR 333 (head https://gitea.com/gitnex/GitNex.git:304-popup-menus-to-bottomsheets) from remote 'main' 2020/04/02 00:24:56 Failed to run app with [tea pr checkout 333]: error creating SSH agent: "SSH agent requested but SSH_AUTH_SOCK not-specified" ```
6543 requested changes 3 years ago
Dismissed
6543 left a comment
Collaborator

a realy nice to have featur ... but I hit an bug

a realy nice to have featur ... but I hit an bug
Poster
Collaborator

@6543 could you post the result of the following command? I suspect your SSH agent is not set up correctly, which wouldn't really be a bug in tea.

echo "pid $SSH_AGENT_PID sock $SSH_AUTH_SOCK"
@6543 could you post the result of the following command? I suspect your SSH agent is not set up correctly, which wouldn't really be a bug in `tea`. ``` echo "pid $SSH_AGENT_PID sock $SSH_AUTH_SOCK" ```
Collaborator

@6543 could you post the result of the following command? I suspect your SSH agent is not set up correctly, which wouldn't really be a bug in tea.

echo "pid $SSH_AGENT_PID sock $SSH_AUTH_SOCK"

This vars are not set at all

EDIT: so it need a ssh agent running in the background :/

> @6543 could you post the result of the following command? I suspect your SSH agent is not set up correctly, which wouldn't really be a bug in `tea`. > > ``` > echo "pid $SSH_AGENT_PID sock $SSH_AUTH_SOCK" > ``` This vars are not set at all EDIT: so it need a ssh agent running in the background :/
Collaborator

if I start ssh-agent and export this vars I get this:

2020/04/05 00:40:55 Failed to run app with [tea pr checkout 139]: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain
if I start ssh-agent and export this vars I get this: ``` 2020/04/05 00:40:55 Failed to run app with [tea pr checkout 139]: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain ```
Poster
Collaborator

Seems like it selects the wrong key.

I could specify a key to use in repo.Push(). Then no ssh-agent is needed, but the default key ~/.ssh/id_rsa will not always be the right one.
Instead we could add a config option for a login to specify an ssh key, defaulting to id_rsa.

Seems like it selects the wrong key. I could specify a key to use in `repo.Push()`. Then no ssh-agent is needed, but the default key `~/.ssh/id_rsa` will not always be the right one. Instead we could add a config option for a login to specify an ssh key, defaulting to `id_rsa`.
Collaborator

@noerw why not fallback -> use ssh-agent and if not afailable fall back to configurable ssh key witch by default use ~/.ssh/id_rsa or is this to mouch?
if so I'm for reading the key direct

@noerw why not fallback -> use ssh-agent and if not afailable fall back to configurable ssh key witch by default use `~/.ssh/id_rsa` or is this to mouch? if so I'm for reading the key direct
Poster
Collaborator

@6543 I added authentication for https & ssh (trying ssh-agent, and then falling back to reading a key manually), so it should work for your setup now as well.

(Logins now store username & path to ssh key, both config values are optional)

Edit:
It seems like gitea does not support Token Auth for git pushes over https? If it does (I couldn't get it to work), one could use the tea token instead of prompting users for credentials

@6543 I added authentication for https & ssh (trying ssh-agent, and then falling back to reading a key manually), so it should work for your setup now as well. (Logins now store username & path to ssh key, both config values are optional) Edit: It seems like gitea does not support Token Auth for git pushes over https? If it does (I couldn't get it to work), one could use the tea token instead of prompting users for credentials
Collaborator

@noerw It asks for authentification on checkout for a https conection every time

@noerw It asks for authentification on checkout for a https conection every time
Collaborator

had this issue only on a single repo - if it happen's again I will debug it cant find any issues - thanks @noerw this subcomand is a huge help (testing this branch since 1w 👍 )

had this issue only on a single repo - if it happen's again I will debug it cant find any issues - thanks @noerw this subcomand is a huge help (testing this branch since 1w :+1: )
6543 approved these changes 2 years ago
Dismissed
Collaborator

@noerw can you "update" this pull ?

@noerw can you "update" this pull ?
lunny approved these changes 2 years ago
Dismissed
lunny merged commit 4cda7e0299 into master 2 years ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 4cda7e0299.
Sign in to join this conversation.
Loading…
There is no content yet.