Implementation of the `gf` command, v2 (RFC)

[Shugyousha]

This is a rebased continuation of PR https://github.com/martanne/vis/pull/502. I have not closed the other one yet because we may have to cross-reference some things from there.

This implementation uses lpeg to detect the filename and the offset pattern. The implementation is still dependent on the text_object_longword function and I am not sure how we can get rid of this dependency. Even when using lpeg we have to know where to start searching for the lpeg pattern. Since the current cursor position could be in the middle of a file name, we have a sort of chicken-and-egg problem. We need a way to find out where the filename begins before we can start searching for the file name lpeg pattern...

In the earlier implementation we discussed reusing the sam.c parser (potentially through vis:command) in order to parse the offset string after the file name. I do the (simplified) parsing using lpeg in the lua code because we would have to find at least the beginning and end of the "filename + offset" string there anyways before we can pass it to vis:command.

The current version also parses offset patterns like 'filename.c:/somestring'. The '/somestring', if found, is passed to vis:command which causes vis to move the cursor to the first occurrence of 'somestring' in the file (which we want) and then changes to visual mode (which we don't want).

I haven't yet implemented the parsing of quoted filenames. I assume the parser should parse something like "file with horrible spaces.c":123 and not "file with horrible spaces.c:123" (or both?).

Open questions:

• Should the Address parsing code in sam.c be used (through vis:command) to do the parsing or is lpeg sufficient?
• How should we move the cursor to the first occurrence of "pattern"? Do we have to expose some search functionality through lua (if not available already)?
• Do we want to merge a (cleaned-up) version of this implementation already or should we first decide on the points above and then implement another version?
• Should we replace other filename parsing code with a shared lua implementation?

[realwhz]

Hi Shugyousha, is this supposed to work on quoted filename, like "filename", or 'filename'?

[Shugyousha]

On Tue, May 30, 2017 at 5:41 AM, Hongzheng Wang [email protected] wrote:

Hi Shugyousha, is this supposed to work on quoted filename, like "filename", or 'filename'?

In case of "filename" or 'filename' it should work but "file name" won't.

After there has been a decision of whether 'file name:234' or 'file name':234 (or both?) should be parseable I can look into it.

[martanne]

A couple of comments:

• Can you use pure lpeg patterns instead of relying on the re module? You can also reuse some patterns provided by the lexer module.
• Instead of the text object function try using file:match_at. That should allow you to use things like the delimited_range function of the lexer module.

Regarding the parsing, how does acme separate the file name from a possible address?

[Shugyousha]

On Tue, May 30, 2017 at 01:02:28PM -0700, Marc André Tanner wrote:

• Can you use pure lpeg patterns instead of relying on the re module? You can also reuse some patterns provided by the lexermodule.

Ok, I can use something like this

local l = require('lexer') local lpeg = vis.lpeg local namepat = l.any - lpeg.S('"'\t\v\f\n\r ')

This still doesn't cover the quoted-filename case but I can use delimited_range for that.

• Instead of the text object function try using file:match_at. That should allow you to use things like the delimited_range function of the lexer module.

I don't think match_at will work here. match_at expects to be given a position to start searching at, a pattern to search for and an optional search horizon. If the cursor is within a file name and you pass a search horizon that is larger than the number of characters between the cursor position and the start of the filename, you will miss the file name and may end up matching the preceding token because the search will always start at pos - horizon. If your chosen search horizon is too small however you won't match the whole file name.

The only way I could think of to make sure to match the whole file name is to move the cursor back from pos to the beginning of the potential filename (by checking each preceding character if it may be part of the file name or not).

Also, when using match_at, it wouldn't be possible to both match the filename and the offset in one call since the function only returns one range (and not the start and end of both the filename part and the address-within-the-file part like we do with the current :match call).

Regarding the parsing, how does acme separate the file name from a possible address?

I looked into it through

https://github.com/Harvey-OS/harvey/blob/6533db6114e97d5687bdb6a0f94136c98d009408/sys/src/cmd/acme/edit.c

and

https://github.com/Harvey-OS/harvey/blob/52a059d54222c030241de2359803d1d6d7a40ecb/sys/src/cmd/acme/ecmd.c

(Harvey OS is the only Plan9 source I could find that was still online)

but I had issues determing how the file name is being parsed. The parsing of the address can be found at

https://github.com/Harvey-OS/harvey/blob/52a059d54222c030241de2359803d1d6d7a40ecb/sys/src/cmd/acme/ecmd.c#L998

Some of the possible Acme filename addresses are shown here:

https://youtu.be/dP1xVpMPn8M?t=9m9s

I strongly suspect that Acme just consideres any string up to a space char as a file name but I am not sure.

Do you want to support quoted filenames? If so, do you want to support

"file with horrible spaces.c":123

or

"file with horrible spaces.c:123"

or both?

[martanne]

I think you misunderstand how match_at is supposed to work (and yes the documentation probably sucks). It will start at pos - horizon and repeatedly attempt to match the pattern as long as the end of the matched range lies before pos.

The following text object should be a starting point:

vis:textobject_new("gf", function(win, pos)
        local l = vis.lexers
        local dq_str = l.delimited_range('"', true)
        local sq_str = l.delimited_range("'", true)
        local include = l.delimited_range("<>", true, true, true)
        local filename = dq_str + sq_str + include + l.word
        return win.file:match_at(filename, pos) 
end, "Filename")

Upon successful match you will have to get the range with file:content, then possibly remove the delimiting symbols etc. Also the current pattern doesn't include . which you probably want for file names.

I will later check what acme does.

[Shugyousha]

On Sun, Jun 18, 2017 at 02:52:24PM -0700, Marc André Tanner wrote:

I think you misunderstand how match_at is supposed to work (and yes the documentation probably sucks). It will start at pos - horizon and repeatedly attempt to match the pattern as long as the end of the matched range lies before pos.

That's how I understood it to work as well (though the matching of the pattern will continue until pos+horizon, right?) but I don't think this will be sufficient to extract the filename under the cursor position. Let me illustrate:

if library.Open("filena|me.ext") {

the | in this text indicates the cursor position. The match_at function with a pattern for filenames will only match the filename within the double quotes (which is what we want to pass to vis:open) if the search horizon is smaller than 9. If the horizon is bigger than that, the search will start before the actual filename we want to match begins. If the search horizon is 14 for example the search will start at the position indicated with an exclamation mark below.

if librar!y.Open("filena|me.ext") {

The filename pattern would then match a string like 'y.Open' (provided we don't treat the bracket as a valid filename character).

Since we can never know which is the search horizon that we want we need a way to find the position at which we want to start the pattern matching.

Does that make sense?

The following text object should be a starting point:

vis:textobject_new("gf", function(win, pos)
        local l = vis.lexers
        local dq_str = l.delimited_range('"', true)
        local sq_str = l.delimited_range("'", true)
        local include = l.delimited_range("<>", true, true, true)
        local filename = dq_str + sq_str + include + l.word
        return win.file:match_at(filename, pos) 
end, "Filename")

Thanks! Extracting a quoted filename should be very well possible using lpeg. I will look into the best way to do it.

[martanne]

If the search horizon is 14 for example the search will start at the position indicated with an exclamation mark below.

if librar!y.Open("filena|me.ext") {

The filename pattern would then match a string like 'y.Open'

That might be the case, but it won't be the return value of match_at because it doesn't include the queried position (denoted by | in your example). This is what I meant with "repeatedly attempt to match the pattern as long as the end of the matched range lies before pos". Hence it will retry starting at the open parenthesis, this time matching "filename.ext" which includes the desired position.

[Shugyousha]

On Mon, Jun 19, 2017 at 9:14 PM, Marc André Tanner [email protected] wrote:

If the search horizon is 14 for example the search will start at the position indicated with an exclamation mark below.

if librar!y.Open("filena|me.ext") {

The filename pattern would then match a string like 'y.Open'

That might be the case, but it won't be the return value of match_at because it doesn't include the queried position (denoted by | in your example). This is what i meant with "repeatedly attempt to match the pattern as long as the end of the matched range lies before pos". Hence it will retry starting at the open parenthesis, this time matching "filename.ext" which includes the desired position.

That means the first match that includes pos will be returned, correct?

We would still have to call 'match_at' twice though, once to get the filename and once to get the address part (if present). Unless you want to separate the filename from the address without using lpeg (i. e. just searching for the separator between filename and address in the "filename+address" returned by "match_at").

[Shugyousha]

I have reimplemented the functionality using match_at and delimited_range.

To detect the offset commands (":123" or ":/searchstring") I just use the index of the first colon which will break easily if the filename contains a colon. Getting the index of the last colon character would be more robust.

Currently only the

"filename with spaces.txt:123"

pattern will work but not the following one.

"filename with spaces.txt":123

[Earnestly]

I'm curious. Under what circumstances would a tool, be it a compiler or even grep, output filenames wrapped in quotes? Is the concern focused on people creating filenames with embedded quotes?

In the latter case a real world example would be:

% grep -Hn . '"foo"'
"foo":1:potatoes

As for filenames containing :. Realise that this is essentially not supported in UNIX. Even though you're allowed to have colons in filenames (and directories), no infrastructure takes acount for it.

For exmaple, execve(3) makes no attempt to escape (or even understand escaped) colons in PATH when attempting to execute non-absolute paths.

(It's a bit like how spaces in filenames are supported but that means essentially nothing because every single Makefile will break when used. E.g. Amost every install target which does install -Dm0755 $(bin) $(DESTDIR)$(PREFIX)$(bindir)/$(bin) will break if any of those expansions contain a space.)

Basically what I'm trying to ask is; Does gf/gF need to be this complicated?

Can edge-cases be handled after the fact if the code is kept "changable" (as Wirth would have put it) instead?

[Shugyousha]

On Thu, Nov 09, 2017 at 12:17:46PM +0000, Earnestly wrote:

[...]

As for filenames containing ':', realise that this is essentially not supported under UNIX. Even though you're allowed to have colons in filenames (and directories), no infrastructure takes acount for it.

For exmaple, execve(3) makes no attempt to escape (or even understand escaped) colons in PATH when attempting to execute non-absolute paths.

Basically what I'm trying to ask is; Does gf/gF need to be this complicated?

I think the current state is complicated enough but it should be workable.

I removed some more unneeded code. I would like to move this PR forward because I miss this functionality in vis. Without martanne telling me what I should do before we can get it merged it will be difficult though...

[mcepl]

I have rearranged this branch into the patch and included it into my build of vis. When running gf command on the line:

Patch0:         Shugyousha-luagfv2.patch

(with the cursor on word Shugyousha) I get new split in vis with this error message:

/usr/share/vis/plugins/open-file-under-cursor.lua:11: attempt to index a nil value (field 'cursor')

Any ideas, how to debug?

[Shugyousha]

Hi Matěj

Matěj Cepl [email protected] wrote:

I have rearranged this branch into the patch and included it into my build of vis. When running gf command on the line:

Patch0:         Shugyousha-luagfv2.patch

(with the cursor on word Shugyousha) I get new split in vis with this error message:

/usr/share/vis/plugins/open-file-under-cursor.lua:11: attempt to index a nil value (field 'cursor')

It looks like the lua API changed and now uses a selection object instead of a cursor one.

I have rebased the code and pushed a commit that should fix this issue.

Cheers,

Silvan

[mcepl]

It works perfectly now. Thank you.

[mcepl]

Hmm, it seems that this patch somehow leads to #827.

[mcepl]

Rebase on the top of the current master has been sent to ~martanne/[email protected] list.

[mcepl]

When I added this patch to my build, I got https://github.com/martanne/vis/issues/827 right back.

[mcepl]

When I added this patch to my build, I got #827 right back.

Because I am an idiot and I have rebased the patch incorrectly on the current master.

This is the correct version of the patch

[Shugyousha]

This is the correct version of the patch

It's good to have this confirmation! I quickly tried to reproduce this failure locally and I couldn't so I was pushing this investigation off some more

my laziness prevails again! :P

[mcepl]

This seems to be breaking Lua tests.

_log.txt

[Shugyousha]

I have sent a version with only the necessary changes (fixing a deprecation warning on the way) to the mailing list: https://lists.sr.ht/~martanne/devel/patches/44573

This seems to be breaking Lua tests.

When I run the tests locally, I only get one failure that seems to be present on the master branch as well (which is this test Running test ./commands/loop-empty-match2 with vis ... FAIL).

I will have a look at what the sr.ht build service reports (assuming that it tries to build this automatically).

[mcepl]

This PR could be closed, couldn’t it?

[Shugyousha]

On September 11, 2023 9:33:53 AM GMT+02:00, "Matěj Cepl" @.***> wrote:

This PR could be closed, couldn’t it?

Yes! Let's close it and continue the discussion on the sr.ht mailing list.

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[mcepl]

Hint: don't you have a [Close] button?

[Shugyousha]

On September 11, 2023 4:56:36 PM GMT+02:00, "Matěj Cepl" @.***> wrote:

Hint: don't you have a [Close] button?

hinthint,nudgenudge :P

I'll close it once I'm back at my computer

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[Shugyousha]

This change is being discussed on the mailing list which means this PR can now be closed.