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

Merged
lunny merged 20 commits from noerw/tea:issue-97/pulls-clean into master 2020-04-19 03:09:08 +00:00
Member

fixes #93, #97, #107

fixes #93, #97, #107
lunny reviewed 2020-03-08 01:32:07 +00:00
Dismissed
cmd/pulls.go Outdated
@ -73,0 +174,4 @@
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.
Author
Member

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 2020-03-08 01:32:36 +00:00
Dismissed
@ -0,0 +1,107 @@
package git
Owner

copyright head.

copyright head.
lunny added the
kind
feature
label 2020-03-08 12:30:40 +00:00
lunny added this to the v0.3.0 milestone 2020-03-08 12:30:45 +00:00
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) 2020-03-11 20:04:40 +00:00
Author
Member

@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 2020-03-29 23:19:11 +00:00
Dismissed
@ -62,3 +55,1 @@
return err
}
idx, err := argToIndex(index)
Owner

no err check

no err check
Author
Member

thanks, resolved

thanks, resolved
6543 reviewed 2020-03-31 22:55:53 +00:00
Dismissed
@ -14,3 +19,3 @@
)
// CmdPulls represents to login a gitea server.
// CmdPulls is the main command to operate on PRs
Owner

good catch!

good catch!
6543 reviewed 2020-03-31 23:03:57 +00:00
Dismissed
@ -91,0 +134,4 @@
}
// verify related remote is in local repo, otherwise add it
newRemoteName := fmt.Sprintf("pulls/%v", pr.Head.Repository.Owner.UserName)
Owner

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 2020-03-31 23:07:43 +00:00
Dismissed
@ -91,0 +202,4 @@
return fmt.Errorf("PR is still open, won't delete branches")
}
// check if the feature branch is ours:
Owner

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

can you move L205-207 too current 213 ...?
6543 approved these changes 2020-03-31 23:28:59 +00:00
Dismissed
6543 left a comment
Owner

a few questions ... but looks good

a few questions ... but looks good
@ -0,0 +20,4 @@
localBranchRefName := git_plumbing.NewBranchReferenceName(localBranchName)
err := r.CreateBranch(&git_config.Branch{
Name: localBranchName,
Merge: git_plumbing.NewBranchReferenceName(remoteBranchName), // FIXME: should be remoteBranchName
Owner

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 ```
Author
Member

@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.
Author
Member

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
Owner
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 2020-04-01 22:26:39 +00:00
Dismissed
6543 left a comment
Owner

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

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

@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" ```
Owner

@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 :/
Owner

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 ```
Author
Member

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`.
Owner

@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
Author
Member

@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
Owner

@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
Owner

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 2020-04-18 00:25:05 +00:00
Dismissed
Owner

@noerw can you "update" this pull ?

@noerw can you "update" this pull ?
lunny approved these changes 2020-04-19 03:08:45 +00:00
Dismissed
lunny merged commit 4cda7e0299 into master 2020-04-19 03:09:08 +00:00
Sign in to join this conversation.
No description provided.