Skip to content

Fix errors in GFM slash commands matcher

username-removed-803846 requested to merge YarNayar/gitlab-ce:gfm-autocomplete-fixes into master

What does this MR do?

It fixes 2 errors in GFM slash command matcher.

Description

Here's 2 moments which in my point of view are just errors, I'd like to discuss it:

1.Regexp in GFM slash command matcher:

( https://gitlab.com/gitlab-org/gitlab-ce/blob/80ad1c62991fd5e3fcdc0e9d803d4df19bef9bf2/app/assets/javascripts/gfm_auto_complete.js.es6#L86 )

Look at (?![" + atSymbolsWithBar + "]) part. (This part basically means "match" if not followed by another slash command symbol.)

The atSymbolsWithBar is all the At.js "at symbols" concatenated with | sign. So it is :|~|@|%.... The thing is that it's placed inside character set, and pipe sign | doesn't have any special meaning inside character set, so it just pipe symbol inside set is added several times without reason.

It should be either just (?![" + atSymbolsWithoutBar + "]) OR (?!" + atSymbolsWithBar + "). Second option is better because it will work in case "at symbol" contains more than one symbol.

2.The return statement in the same function:

https://gitlab.com/gitlab-org/gitlab-ce/blob/80ad1c62991fd5e3fcdc0e9d803d4df19bef9bf2/app/assets/javascripts/gfm_auto_complete.js.es6#L91

Couple of days back it was just return match[2] || match[1], but now it became (and it scares me) the return (match[1] || match[1] === "") ? match[1] : match[2]; which meaning I just don't get.

Initially, the match[2] || match[1] alternate return came from original At source when they have 2 pairs of capturing group: (https://github.com/ichord/At.js/blob/330a42714c4bb5046723b21290a782075792dafe/src/default.coffee#L56 )

But we weren't having the second capturing parenthesis for a while, but match[2] still was here.

Even now, when second pair of parenthensis is back, there is we have only one MEANINGFUL capturing parenthesis pair in the reg exp: (([A-Za-z" + _a + "-" + _y + "0-9_\'\.\+\-]|[^\\x00-\\x7a])*) , which is the first pair, the second pair is used for grouping purpose, not for capturing.

So, it should be just return match[1]

Last time it was changed in !8729 (merged), so I'd be glad if @filipa and @samrose3 and others who interested in subject join this discussion and elaborate on it.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Merge request reports