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

Escape nicks during nick completion when expand_escapes is enabled #709

Merged
merged 1 commit into from Jun 23, 2017

Conversation

Projects
None yet
4 participants
@osm
Copy link
Contributor

commented May 25, 2017

This commit fixes issue #693.

I am not sure what the protocol for sending pull requests to this project are, so I just send it. Let me know if I have done something stupid so I can fix it, my C skills feels a bit rusty.

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

hi, thanks for getting involved! I thought there was a function to do escaping already implemented somewhere in the source code, I will search for it tomorrow. And please remove your vim swap file from the commit ;-)

@osm

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Hi,
Yeah you are of course right, I just found the escape_string function (and sorry about the swap file :-)

/* only escape if we find either \\r or \\n in the nick */
if (l->data != NULL &&
(strstr(l->data, "\\r") != NULL ||
strstr(l->data, "\\n") != NULL)) {

This comment has been minimized.

Copy link
@dequis

dequis May 26, 2017

Member

expand_escapes/expand_escape handle much more than just \r and \n, there's \xFF, \cA, \777 and so on.

This comment has been minimized.

Copy link
@osm

osm May 26, 2017

Author Contributor

Yeah, my bad. So what would you prefer me to do here? A new function that handles just \r and \n?

This comment has been minimized.

Copy link
@osm

osm May 26, 2017

Author Contributor

I added a new function now with the latest commit that takes a pattern and escapes all occurrences of the pattern in the submitted string. I hope this is an OK solution :)

This comment has been minimized.

Copy link
@dequis

dequis May 27, 2017

Member

Uh, sorry, I meant the opposite. You shouldn't be limiting this to just \r and \n

This comment has been minimized.

Copy link
@osm

osm May 28, 2017

Author Contributor

Ah, yeah I was thrown by the initial issue, so I didn't think about that, rookie mistake :-).

I have removed the escape_pattern func now and the if statement you pointed out, so now it uses escape_string on all nicks.

I guess the only question now is the one below here, if you would like me to change where the escaping occurs or not. Let me know if you want me to fix this as well.

@dequis

This comment has been minimized.

Copy link
Member

commented May 26, 2017

What's up with your commit timestamps? They seem to be off by a whole day at least.

I'm not sure it's a good idea to iterate over the whole completion list. It just seems like a bad place to introduce the escaping (also, there are other places in the same function that manipulate completion lists and would require the same thing). Would be better to do it at a point where the completion word is chosen from the list and it's added to the input box.

@osm

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Thanks for the feedback, much appreciated.

I was developing inside a virtual box running OpenBSD, apparently it didn't handle the time so well. Will fix it before I commit anything else.

I agree that it is not optimal to iterate over the whole list, I decided to do this because it was the only place I could find that only handled nick completions for messages in a channel. But this might just be because I'm too unfamiliar with your code base, would you mind pointing me to a file and line where you think it would be more suitable to place the escape?

I am not sure that we need to escape the nick somewhere else though. For example, when you do a /msg you don't want to escape the nick. I think it is only valid to escape the nick when sending something in a channel, right?

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

hi, just to clarify, we need to escape the nicks when they are inserted on the input line, but not when they are part of a command (like /query) . I haven't checked (yet) if your code does that or not

@osm

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

Hi,

Yeah I only tested with the /msg command and that worked, but just tried with /ctcp and then it applied my escaping, so that's not good. I will try to find another place to put the escaping instead.

@osm

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

So, here's a new stab on doing the escaping the right way, thanks for your patience :-)

I have done it like this now:

  1. Moved the escaping to the word_complete function and removed my escape_nicks function I created in an earlier commit.
  2. Stopped iterating over the complete list of nicks and just escape the word that will be displayed in the input line.
  3. Only escape words if the line doesn't begin with a / (this is how I determine if it's a command or not).
  4. Added a check to the expand_escapes function so it doesn't expand our previously escaped nicks (or other text).

Question: Would you like me to create a new PR with just this one last commit if this last attempt is OK? I'm not sure how github works with merges, but if all my failed attempts will show up in the log it will be cleaner if I just abandon this PR and create a new one, what do you think?

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2017

Hi, I still need to take a look at your changes but thanks for the effort. It sounds like you found a good solution. In the meantime you can grep the source for cmdchars, which you need to check instead of /. And once we're all happy with your code, you can "force push" your final commit into this PR, thereby removing the earlier tries from history

@@ -726,7 +726,6 @@ int expand_escape(const char **data)
}
}

/* Escape all '"', "'" and '\' chars with '\' */

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy Jun 4, 2017

Member

What happened to this comment?

This comment has been minimized.

Copy link
@osm

osm Jun 5, 2017

Author Contributor

Ah, that was removed by accident. I have reverted this in the latest commit.

@@ -241,28 +241,37 @@ char *word_complete(WINDOW_REC *window, const char *line, int *pos, int erase, i
if (complist == NULL)
return NULL;

/* escape if the word doesn't begin with '/' and expand_escapes are turned on */
data = *line != '/' && settings_get_bool("expand_escapes") ?

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy Jun 4, 2017

Member

Are you trying to check whether line is a command?
If so you should honour the cmdchars option instead of checking for / only.

This comment has been minimized.

Copy link
@osm

osm Jun 5, 2017

Author Contributor

Fixed in the latest commit.

/* word completed */
*pos = startpos+strlen(complist->data);
*pos = startpos+strlen(data);

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy Jun 4, 2017

Member

Spaces around the +, let it breathe!

This comment has been minimized.

Copy link
@osm

osm Jun 5, 2017

Author Contributor

Fixed.

last_line_pos = *pos;
g_free_not_null(last_line);
last_line = g_strdup(result->str);

ret = result->str;
g_string_free(result, FALSE);

/* free data if we have been escaping something */
if (data != complist->data)

This comment has been minimized.

Copy link
@LemonBoy

LemonBoy Jun 4, 2017

Member

I'm not a great fan of such pointer comparisons, some other places in the code handle this situation by adding a g_strdup and always call g_free when they're done.

This comment has been minimized.

Copy link
@osm

osm Jun 5, 2017

Author Contributor

Should also be fixed now :)

@osm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

Everything should be fixed now according to the comments from @LemonBoy and @ailin-nemui. If you think everything looks OK now I will squash all commits into one and force push it again so I don't mess up the commit log completely :)

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

can you explain to me again how (4) Added a check to the expand_escapes function works, and why it is necessary?

@osm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

The check added in expand_escapes checks if there are two backslashes in a row, if this is the case it indicates that we deliberately have been escaping something, so it moves past the two backslashes and continues to expand any other \r or \n that might exist.

Consider the following text that is going to be sent to a channel:

foo\\nbar: hello\nworld

Without this check the following message to a channel would produce this output:

21:03 < osm> foo
21:03 < osm> bar: hello
21:03 < osm> world

With the check the output will be like this, which I think is better (even though the nick will be incorrectly padded with an extra \, but I don't think there is any way around this).

21:04 < osm> foo\\nbar: hello
21:04 < osm> world
@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

hi @osm I think there is something odd about it, there is probably another bug somewhere which expands the codes multiple times(?), so in any case I think we need to fix them independently, but your code for the tab completion seems good

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2017

maybe you can test my patch (and remove that bit from your code)

@osm osm force-pushed the osm:master branch from e51dced to 12d671a Jun 21, 2017

@osm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2017

I have removed my fix for the #723 PR that you created now. I have also tested #723 and verified that it worked together with this PR and as you can see I squashed all the previous commits into one nice looking thing :)

@osm osm changed the title Escape \r and \n from nicks when expand_escapes is enabled Escape nicks during nick completion when expand_escapes is enabled Jun 21, 2017

@ailin-nemui ailin-nemui merged commit 28d82c8 into irssi:master Jun 23, 2017

@ailin-nemui

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2017

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.