@b4n approved this pull request.

LGTM! :tada: :partying_face:

04eec181710a1e8537b7f4f83e98658e3cf18907, 2927ec4dbb139528a07209d551897098425f3ebe, 99a993fbfed30075610a30432ffe5ae1c3ae2fc3, 3847b6c2a9e090a3362d35dac51c3859f628cacd and ccd5330a21318a0f39bcaf330dac1188e25ab6aa could be squashed in 353ceff30518ea0291a1f8cae279d66dd0a1c469 if you like, but it's fine with me as is -- and possibly even better, I'm not completely sure which way I like best.

Apart from that, there's basically no change in the non-main part diff, so there's not much to add to v4 :)

I'll also try and take a look at importing upstream bundled regex library to at least get Jenkins happy, and we can always use another dep later -- unless @codebrainz or @eht16 have another solution at hand?

I'd like to propose a import strategy again, that allows future updates via `git merge` or `git subtree pull`, basically what I did for scintilla4 (see #2600). What do you think?

I proposed it in the previous PR already, here are the advantages again:

- Files where we don't make changes always merge fine (for dirs we don't use). - Our changes and upstream changes are merged by git's methods - deletions/renamings can cause tree conflicts which are messy to deal with

See #2600 for a proof-of-concept, it really works well if you ask me. Upstream releases are imported in a side branch (without any changes, plain, unmodified upstream) and we merge that into geany's master. Our modifications to the project are done directly on geany's master and will be preserved due to git's merging.

Independently of @kugel- 's proposal I created a script which updates Geany's ctags from universal ctags. I'm not completely sure if we are ready to do it the way @kugel- proposes - our parsers are still very different, upstream ctags contains many files we are not interested in and personally I don't completely see the advantage of this approach (but I may be missing something).

@kugel- I'm not familiar with subtrees, yet they look tempting. However, here we only use a subset of upstream, so how would we only get what we want? If deleting/renaming is messy, I would expect this to be a huge pain if we have the whole of upstream. And if we only copy a subset of upstream anyway, I start to wonder if it's any better to use a subtree. For the moment I see subtrees tempting *if we can use the upstream repository unmodified*, kind of like a submodule but easier for users. If we need to manually import anyway, or if we lose history, I'm really not sure how it's better than plain copying. We don't actually need any of the power of Git to import changes, we have a very minimal patch that would hopefully disappear one day, and that IMO should stay very visible -- not buried in a years old merge.

I might give this a look to get a feel for it as again I'm not familiar with using those so I might miss some of its advantages though.

I any case though, I don't think that's something that should be done in this PR, or before merging this PR. I'm totally OK with the current state, and if we decide to use a subtree we can do that later, and I really don't want an additional barrier to get that one merged.

@b4n I had a brief look at the regex patches and they look good to me (haven't tried them though) - should I add them to this pull request?

If you're OK with them, yes, please.

...

...we have a very minimal patch that would hopefully disappear one day...

I actually have a patch modifying our tag manager to achieve that - it's not completely trivial and maybe worth considering if we want to do it that way so I'll post it separately once this pull request is merged.

Depending on what the patch you're talking about involves, would you think upstream would accept a mean of selecting which method is used? It actually could make sense to use their hash version when generating tag files so there's no (likely) duplicates, but use plain numbers when generating a TOC, so maybe there could be an option somehow. Just a thought.

@b4n pushed 2 commits.

33fc269ea8d4d03453489ca55a9ca208ec15aae2 Add GNU regex bundle from upstream ctags and use it if missing b1c9096394c9819342747f1215274af469a19483 Add bundled fnmatch from upstream ctags

Depending on what the patch you're talking about involves, would you think upstream would accept a mean of selecting which method is used? It actually could make sense to use their hash version when generating tag files so there's no (likely) duplicates, but use plain numbers when generating a TOC, so maybe there could be an option somehow. Just a thought.

The patch is not something too horrible, something like 80LOCs, but not some trivial one-liner so better to post it separaterly. Basically it just simply goes through the tags and renames the anonymous ones the way we need but in addition, we also have to update scopes where these tags are involved.

Updating upstream ctags is an option too, I was just worried about "polluting" ctags with things which are too specific for Geany use.

And even if we kept the current few-line patch, it isn't a too terrible thing either.

The patch is not something too horrible, something like 80LOCs, but not some trivial one-liner so better to post it separaterly. Basically it just simply goes through the tags and renames the anonymous ones the way we need but in addition, we also have to update scopes where these tags are involved.

OK, I would have expected something like this indeed. The only possibly worrysome part would be not missing any location that has to be updated, in case tag names get used some other places in the future.

Updating upstream ctags is an option too, I was just worried about "polluting" ctags with things which are too specific for Geany use.

Sure, but maybe having more "readable" anonymous names is something that could benefit other consumers of ctags, even CLI ones. There are other tools that use ctags to construct a TOC, and in a TOC I would argue that a readable name is somewhat better. Though, maybe it's not so true because it's only useful when a human *looks* at the TOC, if the TOC is also used to jump to locations across files boundaries then they need to be unique again. I'm kind of worried about how annoying it'd be to do, but maybe that's even something we'd like to do: transform anonymous names when displaying them, yet keeping a fairly unique internal name behind the scenes to avoid any clashes. Well, that's just thoughts as they are coming :)

And even if we kept the current few-line patch, it isn't a too terrible thing either.

Yeah it's no biggie, and we'll roll with that at least for now.

@eht16 yay, thanks! I just committed your fix slightly modified to improve overall ordering of the checks, but it should have the same result.

Confirmed. Your changes work fine !

@eht16 however, did the bundled regex got used, or did your MSYS have `regcomp()`? Same question for `fnmathc()`. If MSYS has it, it's less relevant for us to bundle it, and maybe we should have a way of using that in the Travis builds instead?

``` ... checking fnmatch.h usability... no checking fnmatch.h presence... no checking for fnmatch.h... no ... checking for regcomp... no ... ```

Full build log with all the boring details at https://gist.github.com/eht16/895090eddc23c6d241eb2cbec25159c0#file-build-lo.... At the end of the build log is the result of `make check` with all tests failed :(. But luckily this was just because the sample files have LF line endings and the generated tag files have CRLF. As a quick hack I added `--strip-trailing-cr` to `runner.sh` (https://gist.github.com/eht16/895090eddc23c6d241eb2cbec25159c0#file-runner-s...) and the results looked way better. Still a few tests failed.

I didn't check why the tests fail but here is the log: https://gist.github.com/eht16/895090eddc23c6d241eb2cbec25159c0#file-test-sui...

Obviously, main/e_msoft.h was not used on my system. I did not yet check why (short on time currently).

Do we have the `WIN32` macro defined? See

https://github.com/universal-ctags/ctags/blob/e3505ad72572983190d99cd704e726...

Good idea. Here is Geany's and Ctags' `config.h` on my Windows system: https://gist.github.com/eht16/e15e74e5f7bffebdf5f4ada38077ea59

I used `./autogen.sh && ./configure && make` on MSYS2 instead of the `mk_mingw.mak`. https://github.com/universal-ctags/ctags/blob/master/docs/windows.rst maybe has two interesting pointers: - `--disable-external-sort is a recommended option for Windows builds.` I used this flag to create the `config.h` above - `If you want to build an iconv enabled version, you must specify WITH_ICONV=yes` this is from the section about `mk_mingw.mak`, the Autotools part automatically detects iconv and on my system CTags was built with iconv support; Geany's `config.h` is missing the `HAVE_ICONV` macro

Fun fact regarding your first question: Geany defines `WIN32` whereas Ctags does not :).

Unless you beat me in time by finding the cause, I'll give the two options above a try at the weekend.

@eht Just curious, do you run `make check` when you make Windows releases? I'm wondering if these are new problems or if they existed before. Also:

1. Would you try opening the file in binary mode and check what tests fail when you do? 2. Would you try changing CRLF line ends in the SQL tests that fail to LF to see if this is the reason for the problem? I checked uctags implementation of file reading and CRLF should be handled there correctly so it could be something parser-specific.

No, I don't. I just ran the checks now with this PR as I thought it might help.

The tests fail in the same way for Geany master, so you are absolutely right: it's not new and not related to this PR.

Ctags uses appveyor for CI on Windows so it might be worth considering to use it as well so we can run tests for Windows builds too.

...

1. Would you try opening the file in binary mode and check what tests fail when you do?

No change.

Weird, I don't get it then. This is the place where tags are written and newlines are written using `\n` - so it's a matter of figuring out why `\n` produces CRLF.

...

2. Would you try changing CRLF line ends in the SQL tests that fail to LF to see if this is the reason for the problem? I checked uctags implementation of file reading and CRLF should be handled there correctly so it could be something parser-specific.

This actually helps. I converted the test files of the six failures to LF and the tests passed fine.

Good, at least we know it's the CRLFs. Now only to figure out why. I'm afraid I'm out of ideas so it's your turn with debugging :-(

So I think we should address this separately, maybe after this PR got merged.

Sounds good to me.

The good news is: without the --strip-trailing-cr change, the binary mode actually fixes all the tests except the six failures.

Great! I think changing the mode to binary is such a trivial change that I could addd it as a commit to this pull request, what do you think?

The six failures can be fixed easily by converting the test samples to LF, however.

Yeah, but it should still be fixed. This could possibly mean that any file using CRLF, which is the default EOL on Windows, would get tags generated incorrectly so they may not appear in the symbol tree. Have you tried e.g. some C file with CRLFs, if the tags show up in the sidebar?

No, the line endings of the input files don't matter. Tested with a C and SQL file (random choice) with LF and then again with CRLF line endings and the generated tags are the same.

I don't get it. Basically when you take the SQL tests that fail with CRLF line endings and open them in Geany, I would assume that there will be tags missing in the sidebar.

With the binary mode change we would also alte rthe behavior when writing tags files which then always contain only LF. I think this is fine even on Windows.

In fact, since we are considering 2.0 release and making "breaking" changes, I was thinking we could switch to the ctags tag file format for tag file generation, unit tests and the tag files we ship. We could still keep support for reading the "proprietary" binary format we use now but using a more standard text format would be nicer IMO.

I don't get it. Basically when you take the SQL tests that fail with CRLF line endings and open them in Geany, I would assume that there will be tags missing in the sidebar.

@techee IIUC the problem is with *tag files* that have CRLF line endings, not source files getting parsed. If that's so, it won't change nothing there, at worse we need special handling to support CRLF and LF not to break compatibility.

@b4n all tags files probably need to be UTF-8 since the symbols we generate in real-time are from the "always UTF-8" Scintilla buffer, that way symbols from both will match.

The basic ctags tab separated format should be ok with UTF-8 IIUC. (so long as we are sure its UTF-8 :)

IIUC uctags is also developing a JSON format but its not finalised yet.

And I'm certainly not supporting tagmanager format, its only plus is that we can modify it.

So I'm all for @techee's idea of putting the binary format to soft retirement, but indeed now might be a good time to sit down and think on possible issues it should tackle.

Agreed - two things that come in my mind: 1. We first need to synchronize kind letters with ctags so we interpret kinds from ctags correctly 2. We store our own information to the tags like TMTagType which are useful e.g. for unit tests and we should also be able to do this for the ctags tag format - I think we could introduce our own extension fields for this.

Anyhow, if there are no new failures, I agree we could merge this and figure them out later. I'd like a summary though, because I'm now feeling confused on the state of this on Windows :)

Is there any use of that field apart for testing our mappings? Anyhow, we should not depend on this field, but we sure can add any one we like for our own purposes, we just cannot really depend on that field being present.

No, there should be no need for it apart from the unit tests and we shouldn't add them e.g. when generating tag files (which I'm afraid we won't be able to drop completely until ctags has all the parsers we have - and there are several of them in the infamous c.c parser).

s/a https://github.com/geany/geany/issues/2677#issuecomment-736096680 (I didn't know whether to post here or there, so…)

Merged #2666 into master.

Big Yey! delayed by several month of being too busy on the one hand and too scared on the other hand to open this message in my inbox fearing there would be another batch of review comments requiring another tens-of-patches-long pull requests :-)

Thanks for all your time reviewing this @b4n!