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

Allow -port <num> or irc.host.tld <num> in /server add #703

Merged
merged 5 commits into from Jun 23, 2017

Conversation

Projects
None yet
3 participants
@vague666
Member

vague666 commented May 11, 2017

Also in /server modify
irc.host.tld <num> has preference if both are specified

value = g_hash_table_lookup(optlist, "port");
port = *portstr == '\0' ?
(value != NULL && *value != '\0' ? atoi(value) : DEFAULT_SERVER_ADD_PORT)

This comment has been minimized.

@dequis

dequis May 13, 2017

Member

spaces instead of tabs

@dequis

This comment has been minimized.

Member

dequis commented May 13, 2017

Other than the indentation nit, sounds good.

@dequis

dequis approved these changes May 14, 2017

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented May 14, 2017

afaics this change will unconditionally modify the port even if you do not specify any port, can you double check?

@dequis

This comment has been minimized.

Member

dequis commented May 14, 2017

Hm yeah, so something like changing DEFAULT_SERVER_ADD_PORT into rec ? rec->port : DEFAULT_SERVER_ADD_PORT. But that line has so many ternaries it's better to turn them into proper if blocks at this point.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jun 4, 2017

can you change the ?:?: to a if, it's too unreadable for me

port = atoi(portstr);
else if (value != NULL && *value != '\0')
port = atoi(value);
else if (g_hash_table_lookup(optlist, "tls"))

This comment has been minimized.

@ailin-nemui

ailin-nemui Jun 4, 2017

Contributor

does we need to add "ssl" also for backward compat?

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jun 6, 2017

note to self: test the case of modifying a port and if irssi can handle multiple ports of the same host

@ailin-nemui ailin-nemui removed the auto-merge label Jun 6, 2017

@vague666

This comment has been minimized.

Member

vague666 commented Jun 7, 2017

@ailin-nemui, how did that work? Was it possible to have several servers with the same name but different ports? That's something I haven't tested, but I've tested all scenarios I could think of when it comes to single servers

@vague666

This comment has been minimized.

Member

vague666 commented Jun 7, 2017

There should be a test document that describes a set of operations that should be verified to work if something is changed. It's a big job to create such a document though

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Jun 7, 2017

@vague666 in the past the idea was that you can have the same named server, with different ports, eg. (A) irc.example.net 6660, (B) irc.example.net 6661, and so on, and then you could modify the port of (A) with /server add -port 6662 irc.example.net 6660

but idk if it ever worked (I currently think it didn't)
hypothetical test suite:

/server add irc.example.net 6660
/server add irc.example.net 6661
/server list
irc.example.net 6660
irc.example.net 6661
/server modify -port 6662 irc.example.net 6660
/server list
irc.example.net 6662
irc.example.net 6661
/server modify -port 6667 irc.example.net 6661
/server list
irc.example.net 6662
irc.example.net 6667
@vague666

This comment has been minimized.

Member

vague666 commented Jun 7, 2017

I can't add the same server with another port in 1.0.2

@ailin-nemui ailin-nemui merged commit 9d32636 into irssi:master Jun 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment