repo: remotes family: implement list/get-url/set-url #67

jtagcat · 2022-01-23T17:59:19Z

edit: ready to review

jtagcat · 2022-01-23T18:08:44Z

Because of

Autocomplete
Hirearchy
Following upstream (git bin)

It'd make sense to use RemotesList instead of ListRemotes; the same with RemoteURLGet and RemoteURLSet (used here) vs GetRemoteURL and SetRemoteURL (style in file).

Are style for func names enforced? I didn't follow the exact naming in the same file.

jtagcat · 2022-01-23T18:35:16Z

force-pushes:

moved a part to utils.go; reusing it
fixed mistake in swapping got-want in some assertions
renamed ListRemotes() to RemotesList(), see above

also added utils: stdoutToStringSlice()

jtagcat · 2022-02-09T13:24:37Z

@unknwon PTAL, planning to do similar PRs for additional commands.

unknwon · 2022-02-10T04:43:25Z

It'd make sense to use RemotesList instead of ListRemotes; the same with RemoteURLGet and RemoteURLSet (used here) vs GetRemoteURL and SetRemoteURL (style in file).

Are style for func names enforced? I didn't follow the exact naming in the same file.

No there is no existing enforced naming convention, but I think your idea of being closer to how a user would use Git bin makes total sense 👍

unknwon

Thanks for the PR! Overall looking good 🤩 Mostly convention issues.

repo_remote.go

unknwon · 2022-02-10T04:52:52Z

repo_remote.go

+// RepoRemoteURLSetFirst sets first URL of the remote with given name of the repository in given path.
+func RepoRemoteURLSetFirst(repoPath, name, newurl string, opts ...RemoteURLSetOptions) error {


How is the "first" come into play? Not seeing "first" is used as git args?

uhh yeah so.. repo can have like
origin urls:

type fetch url: foobar1

type push url: foobar2

type fetch url: foobar3

type fetch url: foobar4

^^^ in this case, the first fetch url is foobar1, and first push url is foobar3

In reality, this almost never happens, and even if it does, the 'first' is used elsewhere as well.

It might be a good idea to add a short 'it is almost always first, and if not, you probably don't have to worry', but don't know on how to word it.

Thanks for the context!

Either way I think we do not need explicitly put "first" in our function name because we are basically pass-on the value from Git CLI, and whatever it returns is what we return to the caller.

repo_remote.go

utils.go

unknwon

Sorry about delayed response 😅

repo_remote.go

unknwon · 2022-02-23T08:12:21Z

repo_remote.go

+// RepoRemoteURLSetFirst sets first URL of the remote with given name of the repository in given path.
+func RepoRemoteURLSetFirst(repoPath, name, newurl string, opts ...RemoteURLSetOptions) error {


Thanks for the context!

Either way I think we do not need explicitly put "first" in our function name because we are basically pass-on the value from Git CLI, and whatever it returns is what we return to the caller.

repo_remote.go

jtagcat · 2022-02-24T20:29:25Z

All bool #67 (comment)

I can't see how, but I improved the wording nevertheless.

edit: True: part probably, removed it.

Push bool #67 (comment)

fixed

Either way I think we do not need explicitly put "first" in our function name because we are basically pass-on the value from Git CLI, and whatever it returns is what we return to the caller.

s/SetFirst/Set/g done

Let's all-in to follow how we use git bin!

s/URLGet/GetURL/g, s/URLSet/SetURL/g

also jfyi you can make direct edits in the PR as you see fit

unknwon

Thanks again for the follow up!

repo_remote.go

unknwon · 2022-02-25T03:39:27Z

I made a push in 1e5f3c2, LMK if you have any questions, otherwise I'm going to merge once looks good to you!

jtagcat · 2022-02-25T09:29:18Z

Made a small change for the comment at here: Add bool

jtagcat · 2022-02-25T09:36:38Z

RemoteSetURLDelete seems odd for me, RemoteDeleteURL is better imo (even though it does not exactly follow git-bin)?

repo_remote.go

jtagcat · 2022-02-25T09:45:43Z

suggested refactor out of this PR:

type RemotesOptions struct {
	// The timeout duration before giving up for each shell command execution.
	// The default timeout duration will be used when not supplied.
	Timeout time.Duration
}

You can just do

type DefaultOptions struct {
	// The timeout duration before giving up for each shell command execution.
	// The default timeout duration will be used when not supplied.
	Timeout time.Duration
}

// and for each:
var RemotesOptions DefaultOptions

unknwon · 2022-02-25T09:47:40Z

// and for each:
var RemotesOptions DefaultOptions

How will RemotesOptions be used look like? Also, do you mean type RemotesOptions DefaultOptions?

jtagcat · 2022-02-25T09:49:51Z

How will RemotesOptions be used look like?

It will be the same to the user, if anything is added later, you can change it from DefaultOptions to something different.

Using type RemoteOptions DefaultOptions, it's like a symlink pretty much.

Also, do you mean type RemotesOptions DefaultOptions?

yes, oops

unknwon · 2022-02-25T09:50:55Z

How will RemotesOptions be used look like?

It will be the same to the user, if anything is added later, you can change it from DefaultOptions to something different.

Using type RemoteOptions DefaultOptions, it's like a symlink pretty much.

Also, do you mean type RemotesOptions DefaultOptions?

yes, oops

Yeah, SGTM! Let's do this in another PR.

As for this PR, I think once we bring back the RemoteSetURLAdd, we're good to go!

jtagcat · 2022-02-25T09:51:08Z

Add does not take any regex.

See #67 (comment)

Either adding the comment, or actually doing static checking would be nice.

jtagcat · 2022-02-25T09:52:08Z

As for this PR, I think once we bring back the RemoteSetURLAdd, we're good to go!

okie then

jtagcat · 2022-02-25T10:05:58Z

LGTM @unknwon

repo_remote.go

unknwon · 2022-02-25T10:43:43Z

Let's wait for the CI!

unknwon · 2022-02-25T13:04:56Z

https://github.com/gogs/git-module/releases/tag/v1.2.0 has been tagged for this merge.

jtagcat changed the title ~~repo: add ListRemotes()~~ repo: add ListRemotes() and remote get-url/set-url family Jan 23, 2022

jtagcat added 2 commits January 23, 2022 21:42

repo: add RemotesList()

74b9861

also added utils: stdoutToStringSlice()

repo: implement remote get-url and set-url family

23cd988

jtagcat changed the title ~~repo: add ListRemotes() and remote get-url/set-url family~~ repo: remotes family: add list/get-url/set-url Jan 23, 2022

jtagcat changed the title ~~repo: remotes family: add list/get-url/set-url~~ repo: remotes family: implement list/get-url/set-url Jan 23, 2022

unknwon requested changes Feb 10, 2022

View reviewed changes

suggestions

2765e5a

jtagcat requested a review from unknwon February 11, 2022 16:16

unknwon reviewed Feb 23, 2022

View reviewed changes

suggs 2

9a7ef77

jtagcat requested a review from unknwon February 24, 2022 20:29

unknwon approved these changes Feb 25, 2022

View reviewed changes

repo_remote.go Outdated Show resolved Hide resolved

repo_remote.go Outdated Show resolved Hide resolved

unknwon added 2 commits February 25, 2022 10:58

Update repo_remote.go

a0a5863

Tidy up code

1e5f3c2

fixup: RemoteSetURLOptions Add description

5863f89

unknwon reviewed Feb 25, 2022

View reviewed changes

repo_remote.go Outdated Show resolved Hide resolved

DeleteURL: regex is mandatory

465b1ac

improve comment for SetURL, readd URLAdd

d24c817

unknwon approved these changes Feb 25, 2022

View reviewed changes

repo_remote.go Outdated Show resolved Hide resolved

repo_remote.go Outdated Show resolved Hide resolved

Apply suggestions from code review

f7d5d5a

unknwon merged commit 9a01961 into gogs:master Feb 25, 2022

jtagcat deleted the listremotes branch February 25, 2022 20:28

		// RepoRemoteURLSetFirst sets first URL of the remote with given name of the repository in given path.
		func RepoRemoteURLSetFirst(repoPath, name, newurl string, opts ...RemoteURLSetOptions) error {

repo: remotes family: implement list/get-url/set-url #67

repo: remotes family: implement list/get-url/set-url #67

Uh oh!

Conversation

jtagcat commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtagcat commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtagcat commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtagcat commented Feb 9, 2022

Uh oh!

unknwon commented Feb 10, 2022

Uh oh!

unknwon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

unknwon Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

jtagcat Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

unknwon Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

unknwon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

unknwon Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtagcat commented Feb 24, 2022

Uh oh!

unknwon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

unknwon commented Feb 25, 2022

Uh oh!

jtagcat commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtagcat commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jtagcat commented Feb 25, 2022

Uh oh!

unknwon commented Feb 25, 2022

Uh oh!

jtagcat commented Feb 25, 2022

Uh oh!

unknwon commented Feb 25, 2022

Uh oh!

jtagcat commented Feb 25, 2022

Uh oh!

jtagcat commented Feb 25, 2022

Uh oh!

jtagcat commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtagcat commented Jan 23, 2022 •

edited

Loading

jtagcat commented Jan 23, 2022 •

edited

Loading

jtagcat commented Jan 23, 2022 •

edited

Loading

jtagcat commented Feb 25, 2022 •

edited

Loading

jtagcat commented Feb 25, 2022 •

edited

Loading

jtagcat commented Feb 25, 2022 •

edited

Loading