search -exec · Google Summer of Code

Thanks for the intro @Peter Pronai, looking forward to seeing your project in action! Before you make a bunch of local commits, please remember to submit patches so progress can be incrementally inspected and discussed.

Ideally, you should carve off the most simple patch(es) you can think of and submit them for review. Typically, two "perfect" patches will earn you commit rights so you can do all those commits to the actual repository.

Peter Pronai (May 18 2018 at 15:33):

@starseeker should I send patches of the smaller changes I have so far? it's just some typedefs/additional fields + the c_exec function is implemented, although I would like nicer error handling in it, but that would probably take some larger changes, outside of that specific function.

starseeker (May 19 2018 at 01:07):

starseeker (May 19 2018 at 02:28):

@Peter Pronai At least with GCC 8 locally here, I can now build strict with latest trunk.

Peter Pronai (May 20 2018 at 16:26):

other thing: I think I'm basically done with the librt part of things, so the next thing would be actually changing db_search to actually accept the new arguments (the exec handler and the userdata), but that would mean having to rewrite every call to it, and there seem to be a lot of them

Peter Pronai (May 20 2018 at 16:27):

of course none of them use -exec _now_, so it'd be fine to just pass some NULL pointers, but later that might lead to problems

Peter Pronai (May 20 2018 at 16:29):

alternative: I could write the TCL exec evaluation function for MGED and test it on its own, and I still need to actually deliver on my promise to do TDD but the CMake testing is quite intimidating, so I've been putting it off

Peter Pronai (May 20 2018 at 16:30):

so, I think I'll do a head first dive into the testing framework and set up the tests for the currently existing functionality and then I'll do another dive into the TCL API

starseeker (May 22 2018 at 02:56):

starseeker (May 22 2018 at 02:58):

We need to be able to exec libged commands, which as a starting point would avoid the need to tangle with the Tcl API.

starseeker (May 22 2018 at 02:59):

I'd also suggest prototyping a db_search function signature, so we can think about how we want to set up the API

Peter Pronai (May 22 2018 at 11:52):

@starseeker I already have the part that calls the callback, writing the MGED side exec handler sounded like something I could do relatively easily, but it's a lot more complicated than I thought.

Peter Pronai (May 22 2018 at 14:28):

well, it's not exactly complicated, more like not very well documented. I just have to call a Tcl command with a list of strings as its parameters.

Peter Pronai (May 22 2018 at 14:29):

Peter Pronai (May 25 2018 at 02:32):

@starseeker is calling libged functions directly actually needed? since they get registered as TCL commands, it makes more sense to me to just call them from TCL. That also makes things like calling TCL builtins more straightforward.

starseeker (May 26 2018 at 02:02):

TCL is (eventually) going to be pushed out of the lower level libraries (including libged) and will live only in libtclcad and above. So TCL shouldn't be necessary to the core functioning of the system.

starseeker (May 27 2018 at 03:15):

@Peter Pronai Do you have a "working" -exec option to search? If so, what does your new db_search function callback look like?

starseeker (May 27 2018 at 03:19):

@Peter Pronai You'll need to be able to pass a function callback into db_search and execute it - how are you thinking to set this up? It shouldn't be Tcl specific

Peter Pronai (May 27 2018 at 03:20):

@starseeker not yet, I'm doing one of the parts I've been putting off: changing the function signatures so I can pass the callback and the userdata around. It's almost done, just one definition left to change.

I also looked into where libged's "commands" are used and it does seem to me like they are only used in MGED.

I postponed the MGED side callback because I thought it will be better if I can test it properly with db_search. Testing the function on its own, without db_search being done, would have meant having to figure out a bunch of build system things and that would have just been wasted time.

Peter Pronai (May 27 2018 at 03:21):

@starseeker before I sent my proposal we agreed with @Sean on the following:
callback function takes an int argc, char *argv[], and a void *userdata

Peter Pronai (May 27 2018 at 03:21):

starseeker (May 27 2018 at 03:21):

Peter Pronai (May 27 2018 at 03:22):

the argv is what the command is called with, the '{}' get replaced with the current node's path

Peter Pronai (May 27 2018 at 03:24):

I think this is as abstract as it can usefully get. It supports every argc/argv kind of command reasonable.

starseeker (May 27 2018 at 03:24):

OK, so the -exec "template" will be populated on a per-search-path basis and the function will be execed separately on each path?

Peter Pronai (May 27 2018 at 03:24):

starseeker (May 27 2018 at 03:25):

something like: search / -name "a*.r" -exec "myfunc --special-option1 --special-option2 {}"

Peter Pronai (May 27 2018 at 03:26):

starseeker (May 27 2018 at 03:26):

libged then uses a function table to look up the function in question (translating "myfunc" into an actual function) and the options and {} are assembled on a per path basis as the arguments to that function.

Peter Pronai (May 27 2018 at 03:26):

Peter Pronai (May 27 2018 at 03:27):

libged is not touched directly, Tcl already has the functions registered so it is left up to Tcl to call them

Peter Pronai (May 27 2018 at 03:27):

starseeker (May 27 2018 at 03:28):

That might work for a first cut, but libged will eventually not have any direct knowledge of Tcl

starseeker (May 27 2018 at 03:28):

MGED (or libtclcad) will need to register a callback for handling Tcl processing with libged

Peter Pronai (May 27 2018 at 03:29):

starseeker (May 27 2018 at 03:31):

For now, as long as any command you want to run is known to the Tcl command execution environment, that should work. But my point is that eventually we don't want to assume Tcl in order to run commands. Let's say someone makes a Python version of MGED. Or they want to link to libged and construct their own search commands programattically, then register their own custom exec function callbacks with libged and run them on the search results.

starseeker (May 27 2018 at 03:32):

You can start with Tcl execing in order to get something going for testing, but once that is demonstrated we need to think about how to generalize things.

starseeker (May 27 2018 at 03:33):

(the latter scenario in particular - linking to libged and supplying customer defined exec functions to run - is fairly likely to crop up eventually)

Peter Pronai (May 27 2018 at 03:33):

Hmm, I guess if it's Python, we would have to indicate which parameter is the path, so that it is presented as the right type.

Peter Pronai (May 27 2018 at 03:35):

I guess if the customer links to libged directly, we can provide a pre-made callback that simply searches for commands in the command table and calls them directly. That's even easier than the Tcl evaling.

starseeker (May 27 2018 at 03:35):

Well, if the function callback hook is a generic int my_exec_func(int argc, const char **argv, void *userdata) and the exec string supplied to the -exec search option is broken out into the argv input passed to my_exec_func (after replacing {} with one path) then the only questions are how to match the string "my_exec_func" in the search -exec function to the actual function pointer to call and how to pass all that around

Peter Pronai (May 27 2018 at 03:37):

starseeker (May 27 2018 at 03:37):

right - we need some way (probably in libged) to define a language agnostic "string to exec function pointer" mapping that can be populated, and then either pass that map to the db_search call so it can figure out how to do the exec there, or have libged do the string to pointer breakdown ahead of time and pass the actual exec pointer to db_search

starseeker (May 27 2018 at 03:38):

if you're almost there with a design, go ahead and finish and we can use that as a basis for further discussion

Peter Pronai (May 27 2018 at 03:39):

starseeker (May 27 2018 at 03:39):

I guess we could make the callback function find the "actual" function that will do the work... not sure about that one.

starseeker (May 27 2018 at 03:39):

Peter Pronai (May 27 2018 at 03:39):

also,seeing how I've changed db_search's prototype, there will be a lot of broken calls to it

starseeker (May 27 2018 at 03:40):

don't worry about that too much yet - let's see how you're thinking about changing it before you go too far.

Peter Pronai (May 27 2018 at 03:41):

Peter Pronai (May 27 2018 at 03:42):

Peter Pronai (May 28 2018 at 14:26):

@starseeker so, I'm trying to fit the exec specific parameters into ged_search but it breaks the command tab, so I'm thinking about adding it to struct ged. Is that ok? Is this supposed to be thread safe and stuff?

starseeker (May 28 2018 at 14:26):

Peter Pronai (May 28 2018 at 14:28):

Sean (May 28 2018 at 14:30):

many commands have custom command hooks, so you shouldn't need to modify the struct ged...

starseeker (May 28 2018 at 14:35):

@Peter Pronai I think the way that's going to have to be handled is to have the calling code "wrap up" the to_cmds information into one container struct that can be passed through the generic exec function, and then have an "unpack and run" function that is the actual hook function supplied to search exec for the Tcl commands. In essence, the unpack function will serve as the dispatcher to the to_cmd interface.

starseeker (May 28 2018 at 14:36):

basically, "unpack" will turn the argv string exec generates and the contents of the data structure into a to_cmd call.

Peter Pronai (May 29 2018 at 17:55):

@starseeker where would the client entry point(s) be? There is a to_open_tcl that seems to allow for some userdata.

starseeker (May 30 2018 at 00:50):

Peter Pronai (May 30 2018 at 00:56):

I'm trying to figure out where the client code would do this and which info to wrap

starseeker (May 30 2018 at 01:48):

Ah. @Sean , what would be the "right" way to pass application client data through a libged command call?

starseeker (May 30 2018 at 01:49):

@Peter Pronai in this particular case you could probably use the ged_interp pointer in struct ged for testing purposes - it's not intended to be relied upon (at least, not in that form) but right now I believe that's holding the Tcl interpreter used by the libged commands working with Tcl.

starseeker (May 30 2018 at 01:50):

So a libged db_search call could pass the gedp->ged_interp pointer through the call with a void cast, and then your "run_to_cmd" function would cast it back to a Tcl_Interp before calling the actual to_cmd.

starseeker (May 30 2018 at 01:51):

starseeker (May 30 2018 at 01:56):

Peter Pronai (May 30 2018 at 02:00):

Sean (May 30 2018 at 06:12):

the only way for a librt function to call into libged state without creating an architecture cycle or violating modularity is for librt to take a function pointer AND a data pointer (which both get passed to the function). qsort() is a quintessential example of this.

there are common ways for doing this statefully (e.g., context = db_search_register_callback(mycallback); db_search(context, ...); // data passed internally or explicitly) or stateless (e.g., db_search(mycallback, ...); // data passed internally or explicitly).

Sean (May 30 2018 at 06:13):

Sean (May 30 2018 at 06:16):

which in that pattern could be as simple as a separate db_search_register_data() call

so with a stateful context, something like ctx = db_search_context_create(); db_search_register_exec(ctx, callback); db_search_register_data(ctx, interp);

starseeker (Jun 02 2018 at 01:20):

Peter Pronai (Jun 02 2018 at 02:25):

@starseeker the last few days were a bit hectic so I'm catching up with the coding today, I just finished modifying librt to use a single context struct pointer instead of separate userdata and callback parameters

Peter Pronai (Jun 02 2018 at 02:25):

right now i'm building mged to see where it broke and where i have to change things in libged

Peter Pronai (Jun 05 2018 at 17:39):

% echo 'search * -exec echo yay ";"' | ./bin/mged ../../db/axis.g
yay

but I think it should be printing that multiple times?? not sure what's going on on the Tcl side, I should probably look at existing scripts for commands I should try.

Peter Pronai (Jun 05 2018 at 17:42):

I tried -exec true ";" but it gives an "invalid command name: true" in the log....but still prints every item.....

Peter Pronai (Jun 05 2018 at 18:00):

-exec echo "{}" ";" seems to do the right thing now but it only printed one path, no idea why. but it returned true for everything so exec printed them all

Sean (Jun 05 2018 at 18:02):

Sean (Jun 05 2018 at 18:07):

Might I suggest running in immediate mode: mged f.g obj search . -exec echo yay \;

Sean (Jun 05 2018 at 18:09):

Peter Pronai (Jun 05 2018 at 18:14):

@Sean the -exec true and the current -exec echo I'm trying are without the asterisk

Peter Pronai (Jun 05 2018 at 18:16):

i know that the f_exec in the plan is evaluated the right number of times so it must be something else messing up

Peter Pronai (Jun 05 2018 at 18:17):

this is what echo 'search -exec echo "{}" ";"' | ./bin/mged ../../db/axis.g does:

/axis/x.r/x
X1
X2
X
X.r
...bunch more names

Peter Pronai (Jun 07 2018 at 23:05):

@starseeker ok so I think I know what's going on and it's probably not my code that's wrong

I've noticed that many parts of the code actually "print" into memory, the echo output I see there is the last node in the db that librt looked at, so what's probably happening is:

Peter Pronai (Jun 07 2018 at 23:24):

ah. apparently it might not be broken, just.... really, really weird and counterintuitive

one would think that echo prints to stdout, right? but no, there is a result field in Tcl_Interp

I guess that should be printed after every Tcl_Eval??? this is weird, why would somebody want this??? can't this be done with pipes???

Peter Pronai (Jun 08 2018 at 15:34):

@starseeker well, I added an extra puts() call so now the eval function I wrote takes care of printing, but it still comes after the Tcl output (in this case: the list of found items) unless I explicitly fflush(stdout)

Peter Pronai (Jun 08 2018 at 15:35):

but yeah, that's... about it??? should I send a patch so others can try it as well?

Peter Pronai (Jun 08 2018 at 15:36):

what I think _could_ be done:
the context is currently the last parameter, but leaving the database pointer to be last might be better? it would be pretty easy to switch their order.

Peter Pronai (Jun 08 2018 at 15:37):

BSD find has the -ok variant, which asks before execution, is this needed?
then there is the "{}" "+" version, which adds things at the end. that's just a minor change in the parser

Peter Pronai (Jun 08 2018 at 15:41):

also, find(1) does not have the implicit -print when -exec or -ok are present, leaving printing up to them

however, librt does not actually print, it just returns an array and the interpeter has no easy way to do that, so I think this should be left out to keep things simple??? or the presence of -exec should be communicated towards the client code, so that it can decide if it wants to do implicit printing

Peter Pronai (Jun 08 2018 at 15:42):

oh and of course existing code outside mged that uses db_search beeds to be updated

Sean (Jun 08 2018 at 16:03):

it's not broken -- tcl does exactly what we tell it to do so if the behavior isn't right, it's pretty much on us -- and how it's been set up in mged/libged

Sean (Jun 08 2018 at 16:04):

there is a result field and most commands call Tcl_AppendResult to add to it. some commands clear the result before printing to start fresh, some merely append, some return a partial result and tell tcl more will come later

Peter Pronai (Jun 08 2018 at 22:34):

yeah, figured it out later, but damn that was frustrating. i'm used to I/O working on the standard files, so this was very weird.

Peter Pronai (Jun 08 2018 at 22:35):

so, what should I do with the remaining thingies? what to implement? what to change?

starseeker (Jun 09 2018 at 01:58):

I'd get things working across the system first (update all the db_search calls, add documentation to the man page for search including syntax documentation and examples)

starseeker (Jun 09 2018 at 01:59):

Peter Pronai (Jun 11 2018 at 00:39):

Peter Pronai (Jun 11 2018 at 00:46):

Peter Pronai (Jun 12 2018 at 02:28):

@starseeker ok, got it right this time, the Tcl result is now properly interpreted
I only tested it with search -exec echo yes ";" and search -exec echo no ";" so far, but those work.

starseeker (Jun 12 2018 at 03:05):

Peter Pronai (Jun 12 2018 at 03:07):

Peter Pronai (Jun 12 2018 at 03:08):

starseeker (Jun 12 2018 at 03:08):

Peter Pronai (Jun 12 2018 at 03:14):

well this is... odd... git diff master works but git diff master -- <some files I took from the diff output> produces an empty output

Peter Pronai (Jun 12 2018 at 03:23):

starseeker (Jun 12 2018 at 03:27):

OK, one thing I see right away - you're allocating the db_search_context with bu_malloc, but calling free on it rather than bu_free. I'd recommend for this situation, if you're going to have a db_search_context_create function, you also add a db_search_context_destroy to handle the deallocation.

starseeker (Jun 12 2018 at 03:29):

minor point - mged_db_search_callback has some trailing whitespace in it, which we generally try to avoid.

starseeker (Jun 12 2018 at 03:31):

Peter Pronai (Jun 12 2018 at 03:33):

starseeker (Jun 12 2018 at 03:34):

Peter Pronai (Jun 12 2018 at 03:38):

Peter Pronai (Jun 12 2018 at 03:39):

starseeker (Jun 12 2018 at 03:52):

starseeker (Jun 13 2018 at 01:53):

Sean (Jun 13 2018 at 14:01):

@Peter Pronai if you have it all working, check out the manual page for search (brlman search) and see which of the examples it has at the end can turn into shorter search -exec examples. it's also a good test set in general to make sure everything is working right.

Peter Pronai (Jun 13 2018 at 18:16):

@starseeker I changed the calls to bu_free but I haven't uploaded it yet, I will in a bit
@Sean thanks, I'll check those (and add some examples with -exec)

Peter Pronai (Jun 14 2018 at 20:46):

bu_free complains at runtime about freeing a null pointer, but afaik free(0) is supposed to be a no-op??? does this need an extra check?

starseeker (Jun 16 2018 at 01:12):

Peter Pronai (Jun 18 2018 at 00:44):

i'm trying to do the examples in the manpage but uhh, i'm getting errors and since Tcl is so amazing, it doesn't even tell me where the error comes from

Peter Pronai (Jun 18 2018 at 00:44):

Peter Pronai (Jun 18 2018 at 00:45):

Peter Pronai (Jun 18 2018 at 00:47):

and that's a reduced example, the one in the manpage was bigger and gave the same error

Peter Pronai (Jun 18 2018 at 00:47):

well. i didn't run it directly so maybe this is some weird context sensitive thing

Peter Pronai (Jun 18 2018 at 01:06):

also, man search doesn't work in -c mode apparently? it says man: no manual page /home/rain/Sync/gsoc/build/debug/share/man/mann/search.nged

Peter Pronai (Jun 18 2018 at 01:11):

uhhh. ok. so it works in the original script but as soon as i try to make the loop body into a function with proc it starts complaining when i call the function

Peter Pronai (Jun 18 2018 at 01:12):

Peter Pronai (Jun 18 2018 at 20:40):

uh turns out the full paths i was printing can't actually be used??? this is.... weird.

Peter Pronai (Jun 20 2018 at 00:57):

ok, made another example work with -exec
damn.
global variables affecting seemingly basic commands..... yikes

Peter Pronai (Jun 20 2018 at 00:58):

also now f_exec doesn't use full paths because it turns out that was just some debugging thing maybe? because the Tcl interpeter wanted nothing to do with them

starseeker (Jun 20 2018 at 01:23):

@Peter Pronai There are some commands that will use full paths - the example that comes to mind is arced, although I don't think I have a scripted example of using it handy

starseeker (Jun 20 2018 at 01:23):

"brlman arced" from the command or "man arced" from the MGED command line should give you the arced man page - I'd try a few experiments with it and then see if you can roll it into a search -exec example successfully

Peter Pronai (Jun 20 2018 at 19:57):

Peter Pronai (Jun 20 2018 at 20:10):

Peter Pronai (Jun 21 2018 at 01:36):

starseeker (Jun 21 2018 at 01:47):

That's OK (commands only working with obj or full path respectively) as long as the commands in question do work when given the correct inputs

starseeker (Jun 21 2018 at 01:47):

Peter Pronai (Jun 21 2018 at 02:00):

i think it was "wrong number of arguments sph2.s"
it went away when i quoted the "-"

Peter Pronai (Jun 21 2018 at 02:02):

mged> r foo.r u rcc2.s – sph2.s
 sph2.s
error in number of args!
mged>

Peter Pronai (Jun 21 2018 at 02:02):

Peter Pronai (Jun 21 2018 at 02:04):

mged> r foo.r "u" rcc2.s "–" sph2.s
" sph2.s
Defaulting item number to 1002
Creating region with attrs: region_id=1001, los=100, material_id=1
bad operation:  (0x0) skip member: sph2.s

Peter Pronai (Jun 21 2018 at 03:35):

@starseeker just throwing this out there: why not have a filter command? functional style? can Tcl do that?

Peter Pronai (Jun 21 2018 at 03:36):

Peter Pronai (Jun 23 2018 at 16:08):

I separated the -exec option into an -execs and and -execf one, the former uses the short name and the latter uses the full path.

Peter Pronai (Jun 23 2018 at 16:43):

starseeker (Jun 24 2018 at 14:30):

starseeker (Jun 24 2018 at 14:32):

@Sean What do you think is the "right" thing to do here? Teach commands that use a path to handle the search output paths, have exec "dumb down" the paths somehow for feeding to libged, ...?

starseeker (Jun 24 2018 at 14:33):

@Peter Pronai I'm not sure we're going to want to have two different exec options - that's OK for testing, but I think we'll want to resolve some sort of sane default behavior for all commands that doesn't require user awareness of that.

starseeker (Jun 24 2018 at 14:34):

@Peter Pronai what does "draw" do with full paths? IIRC Bob updated that some years ago to be able to handle full paths, but there again that lead slash might be an issue...

starseeker (Jun 24 2018 at 14:36):

From a command standpoint, it might be as simple as ensuring sane failure behavior if the "wrong" form of the path (short vs. long) is supplied - then an exec with the wrong search path modifier will just not do anything, and the "fix" is to change the path modifier to produce the correct form for the exec in question.

starseeker (Jun 24 2018 at 14:37):

Of course, there's also the situation with multiple exec calls where some expect short paths and some are looking for long paths.

starseeker (Jun 24 2018 at 14:38):

Part of me almost wants to audit the commands and add (where appropriate) the ability to each command to handle full path inputs by "pre-processing" it down to the leaf object, if that's what the command is looking for.

starseeker (Jun 24 2018 at 14:45):

db_string_to_path -> DB_FULL_PATH_CUR_DIR -> dp->d_namep for each string supplied as a "geometry" object to commands that don't work with full paths, or something along those lines...

starseeker (Jun 24 2018 at 14:47):

I'm definitely much more inclined to fix the commands to do the "right" thing than I am to contort search-exec to try to "feed" them properly dumb-ed down inputs

Sean (Jun 24 2018 at 15:42):

free(0) is fine per the standard but is typically wrong or lazy memory management (often the prior), so bu will blather a warning expecting you to make sure the caller knows how much was allocated, how much will get freed, etc.

Sean (Jun 24 2018 at 15:44):

For the r command, I believe your expression is wrong. Don’t put the first op (u).

Sean (Jun 24 2018 at 15:44):

Peter Pronai (Jun 24 2018 at 20:48):

@starseeker in that case I'll go through the commands and see which ones are doing weird things

Peter Pronai (Jun 25 2018 at 06:54):

come to think of it.... the current behaviour might be ok, since the user could just concatenate the short path with some prefix

Peter Pronai (Jun 25 2018 at 07:19):

i'm trying to figure out how but the Tcl concat command adds spaces between things

Peter Pronai (Jun 25 2018 at 07:20):

Peter Pronai (Jun 25 2018 at 08:20):

so this sort of works: search Default -depth 1 -execs regsub bloop Default/bloop "{}" ";"

Peter Pronai (Jun 26 2018 at 16:35):

Peter Pronai (Jun 26 2018 at 21:13):

found another that takes paths: listeval
that one pretty much only makes sense with paths

starseeker (Jun 26 2018 at 22:11):

@Peter Pronai my sense is that we should come up with one or two path processing functions that will "digest" full path and object names and determine what was supplied, then have commands either reject the input (path command given non-path object, for example) or "do the right thing" (e.g. pull the leaf object from a full path and send that forward to the command.)

starseeker (Jun 26 2018 at 22:11):

Updating all the commands to be robust in that regard will be something of a slog, but we can at least establish the correct patterns.

Sean (Jun 26 2018 at 22:20):

there needs to be a general object pathname processing, but that will require at least a case-by-case evaluation. anything that takes an object name should take a pathname, and anything that takes a object name implicitly refers to a path. fortunately, nearly all commands will easily update to supporting either. the ones that don't will have to be considered separately (e.g., maybe we decide to eliminate or generalize the outliers).

Sean (Jun 26 2018 at 22:25):

Peter Pronai (Jul 02 2018 at 00:27):

so, i have a function like db_lookup that uses paths, it's only missing the part where it checks if the path components are actually connected

Peter Pronai (Jul 02 2018 at 00:29):

Peter Pronai (Jul 02 2018 at 22:24):

Sean (Jul 04 2018 at 04:33):

@Peter Pronai can you explain or show this please? we don't want multiple lookup functions if we don't need to have them. adding support for paths to db_lookup() would seem entirely reasonable if it doesn't support them already. it would naturally need to fail lookup if the path was invalid.

Sean (Jul 04 2018 at 04:33):

Sean (Jul 04 2018 at 05:06):

Sean (Jul 04 2018 at 05:12):

I don't think this is the right thing to do here. whether short or full paths should replace {} is already implied via search . vs search / ... there should just be one -exec option regardless.

Sean (Jul 04 2018 at 05:13):

what did you mean by this? invalid paths? problems with commands that don't take paths?

Peter Pronai (Jul 04 2018 at 20:57):

problems with commands that don't take paths and i think the ones that do take paths don't take absolute ones

Peter Pronai (Jul 04 2018 at 21:01):

struct directory *
db_lookup_path(const struct db_i *dbip, const char *_path, int noisy)
{
    struct directory *dir = RT_DIR_NULL;
    size_t i=0;
    size_t j=0;
    char tmp;
    char *path = bu_strdup(_path);

    /* skip leading slashes */
    while (path[i] == '/' && path[i] != '\0')
        i++;
    j=i;
    while (path[j] != '/' && path[j] != '\0')
        j++;
    tmp = path[j];
    path[j] = '\0';
    /* start from the "root" */
    dir = db_lookup(dbip, path + i, noisy);
    if (dir == RT_DIR_NULL)
        goto exit;
    for(;;) {/* FIXME: figure out a better loop condition than `true`?? */
        path[j] = tmp;
        if (path[j] == '\0') /* reached the last segment */
            goto exit;
        i = j + 1;
        while (path[i] == '/' && path[i] != '\0')
            i++;
        j = i;
        while (path[j] != '/' && path[j] != '\0')
            j++;
        tmp = path[j];
        path[j] = '\0';
        if (1) {
            /* path segment connects to dir */
            dir = db_lookup(dbip, path + i, noisy);
        } else {
            bu_log("malformed path"); /* FIXME: write more descriptive message once the connection check is implemented */
            return RT_DIR_NULL;
        }
    }
    exit:
    bu_free(path, "mutable path");
    return dir;
}

Peter Pronai (Jul 04 2018 at 21:03):

Peter Pronai (Jul 04 2018 at 21:05):

the connection thing: should it eg. only go down unions but not intersections?
i think it should consider any connection a valid one and it any filtering is necessary, that could be added later

Peter Pronai (Jul 04 2018 at 21:09):

there were some unexpected pet related troubles at home so i didn't have a lot of time to code :/
tomorrow we're going on that vacation so i should have ample time to code and read more of the source in the car

Sean (Jul 08 2018 at 06:20):

this likely makes more sense. existing db_lookup could be made a HIDDEN/static function renamed to just lookup_object() or something similarly simple, and your new one taking over as db_lookup() assuming the signature matches. it would need to match.

Sean (Jul 08 2018 at 06:23):

Ah, no, I don't think it should be arbitrarily limited by the operation. Stopping doesn't make sense for intersections in particular. The only real difficulty is that there can be multiple objects at a given point in the path (e.g., /path/to/foo/bar might have 'bar' in 'foo' N times, with different matrices on each one). That is only really solved by switching from paths to URI's, and I think that's outside scope for this project.

starseeker (Jul 10 2018 at 00:37):

@Peter Pronai Peter, can you do another version of your patch that doesn't have extra whitespace in it? I don't remember if we say it explicitly in HACKING, but generally we try to avoid trailing whitespace on lines.

Peter Pronai (Jul 10 2018 at 13:27):

Peter Pronai (Jul 11 2018 at 10:13):

well.. i have something that should check connections, but it seems like it just considers everything connected...? that took a lot longer than i hoped for and it still doesn't work. frick.

Sean (Jul 11 2018 at 15:23):

Peter Pronai (Jul 11 2018 at 23:32):

HIDDEN int
connected_directories(const struct db_i *dbip, const struct directory *dpa, const struct directory *dpb)
{
    int ret = 0xdead;
    struct directory *dpachild;
    size_t i;
    struct rt_db_internal interna, internb;
    struct rt_comb_internal *comba;
    size_t true_count, trueish_count;
    struct rt_tree_array *rt_tree_array = NULL;

    if (RT_DIR_NULL == dpa || RT_DIR_NULL == dpb) {
        /* FIXME: panic maybe instead??????? */
        ret = 0;
    } else if (!(dpa->d_flags & RT_DIR_COMB)) {
        ret = 0; /* not a combination, so paths can't start from it */
    } else {
        if (!(dpa->d_flags & RT_DIR_COMB)) {
            ret = 0;
        } else if (rt_db_get_internal(&interna, dpa, dbip, (fastf_t *)NULL, &rt_uniresource) < 0 || rt_db_get_internal(&internb, dpb, dbip, (fastf_t *)NULL, &rt_uniresource) < 0) {
            bu_log("Database read error, aborting");
            ret = 0;
        } else {
            comba = (struct rt_comb_internal *)interna.idb_ptr;
            if (comba->tree) {
                if (db_ck_v4gift_tree(comba->tree) < 0) {
                    db_non_union_push(comba->tree, &rt_uniresource);
                    if (db_ck_v4gift_tree(comba->tree) < 0) {
                        bu_log("Cannot flatten tree for listing");
                        ret = 0;
                        goto exit;
                    }
                }
                trueish_count = db_tree_nleaves(comba->tree);
                if (trueish_count > 0) {
                    rt_tree_array = (struct rt_tree_array *)bu_calloc(trueish_count,sizeof(struct rt_tree_array), "tree list");
                    true_count = (struct rt_tree_array *)db_flatten_tree(
                        rt_tree_array, comba->tree, OP_UNION,
                        1, &rt_uniresource) - rt_tree_array;
                    BU_ASSERT(true_count == trueish_count);
                    comba->tree = TREE_NULL;
                } else {
                    true_count = 0;
                    rt_tree_array = NULL;
                }

                for (i = 0; i < true_count && !ret; i++) {
                    dpachild = db_lookup(dbip, rt_tree_array[i].tl_tree->tr_l.tl_name, LOOKUP_QUIET);
                    if (RT_DIR_NULL != dpachild && 0 == bu_strcmp(dpachild->d_namep, dpb->d_namep)) {
                        ret = 1;
                    }
                    db_free_tree(rt_tree_array[i].tl_tree, &rt_uniresource);
                }

                while (i < true_count) {
                    db_free_tree(rt_tree_array[i].tl_tree, &rt_uniresource);
                    i++;
                }

                if (rt_tree_array)
                    bu_free((char *)rt_tree_array, "printnode: rt_tree_array");

            } else {
                ret = 0;
            }
        }
    }
    exit:
    rt_db_free_internal(&interna);
    rt_db_free_internal(&internb);
    BU_ASSERT(ret != 0xdead);
    return ret;
}


HIDDEN int
connected_path(const struct db_i *dbip, const struct db_full_path *fp)
{
    size_t i;
    const struct directory *dpa, *dpb;
    switch (fp->fp_len) {
        case 0:
        case 1: return 1;
        default:
            dpb = fp->fp_names[0];
            if (RT_DIR_NULL == dpb) return 0;
            for (i=1; i<fp->fp_len-1;i++) {
                dpa = dpb;
                dpb = fp->fp_names[i+1];
                if (! connected_directories(dbip, dpa, dpb))
                    return 0;
            }
    }
    return 1;
}

i couldn't find a way to check the connections starting from a struct directory so i hoped that the tree command's implementation will have something, that right there is a shameless plagiary of _ged_print_node

Peter Pronai (Jul 12 2018 at 18:52):

maybe the problem is that i'm testing it with the wrong file? I've been doing all my tests with the supplied NIST_MBE_PMI_7-10.g since it's pretty big so i thought it should be complex enough. maybe the output of tree just doesn't reflect the connections the way they really are?

Peter Pronai (Jul 12 2018 at 19:01):

the tests that i expected to fail were things like
ls Default/Default or ls Solid.... oh..... OH!!!!!!!!!!!!!!!!

Peter Pronai (Jul 12 2018 at 19:01):

Sean (Jul 12 2018 at 19:08):

@Peter Pronai that's a big file, but it's not a very complex database .. it's just got some big nurbs objects in it. the havoc or m35 models are considerably more complex

Sean (Jul 12 2018 at 19:09):

Peter Pronai (Jul 12 2018 at 21:05):

Peter Pronai (Jul 12 2018 at 21:06):

and ya, i later realized it's pretty simple but i couldn't remember which one the big complex tank was. >_>

Peter Pronai (Jul 12 2018 at 21:06):

Sean (Jul 12 2018 at 21:08):

Sean (Jul 12 2018 at 21:09):

the "goliath.g" model kind of looks like a tank but is actually a tracked mine ... like a land mine that explodes, but it has tracks on it so it can be driven around

Peter Pronai (Jul 12 2018 at 21:15):

it must have been that then. but anyways, Default/Default shouldn't work and it's not just some quirk of the database in question.

Peter Pronai (Jul 12 2018 at 21:25):

Sean (Jul 13 2018 at 16:04):

it does use db_lookup (see src/libged/list.c) but is also path-aware if there's a slash -- there may be some case where object/object resolves wierd

Peter Pronai (Jul 14 2018 at 21:16):

Peter Pronai (Jul 14 2018 at 21:17):

Peter Pronai (Jul 14 2018 at 21:18):

Peter Pronai (Jul 14 2018 at 21:32):

Peter Pronai (Jul 16 2018 at 12:02):

i think i'll write some scripts to convert between the two, so I can work purely with tabs but then I can convert things back. i've tried the official formatter script but it works on files and I want to work with selections as well and that's easier done with a few sed invocations

Cezar (Jul 16 2018 at 12:14):

Peter Pronai (Jul 16 2018 at 12:27):

Peter Pronai (Jul 16 2018 at 13:51):

# convert from the weird indentation to something simpler
|sed 's/    /        /g
s/    / /g'

# revert the previous conversion
|sed 's/        /        /g
s/  /    /g
s/        / /g'

Peter Pronai (Jul 16 2018 at 13:51):

Peter Pronai (Jul 16 2018 at 13:55):

this is also nicer because it's easy to apply to selections in Acme so it's easier to track what I'm changing

Sean (Jul 16 2018 at 14:11):

Sean (Jul 16 2018 at 14:24):

we have plans to convert the entire codebase indentation over. that was waiting on repo migration, but it might make more sense to do it first now that migration is stalled...

starseeker (Jul 17 2018 at 01:22):

@Peter Pronai So on the patches, please separate them out by topic (you've got the older patch with color, edcolor, prcolor changes which I don't (think?) are related to search -exec)

starseeker (Jul 17 2018 at 01:24):

I'm thinking there are three here - the color stuff, the initial search -exec patch, and the follow-up that does the db_lookup changes

starseeker (Jul 17 2018 at 01:25):

@Peter Pronai what's the "something weird" when you include the type from librt in ged/defines.h?

Peter Pronai (Jul 17 2018 at 10:32):

I think I have separated the prcolor stuff out?? The something weird was some compilation error, but I can't remember what exactly.

Peter Pronai (Jul 17 2018 at 10:33):

But yea, there are changes in my branch that aren't directly related and that's probably not good because it's hard to clean up the diff from the master branch.

Peter Pronai (Jul 17 2018 at 14:50):

problem is idk how to cleanly separate those changes and some of them are nice to have..... maybe i could make each change a new feature branch and make one private branch that has all those changes????? idk

Peter Pronai (Jul 17 2018 at 19:05):

In file included from /home/rain/Sync/gsoc/brlcad-code/include/ged/defines.h:37,
                 from /home/rain/Sync/gsoc/brlcad-code/include/analyze.h:34,
                 from /home/rain/Sync/gsoc/brlcad-code/src/libanalyze/api.c:28:
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:141:39: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
                                struct db_i *dbip,
                                       ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:140:39: error: ‘struct directory’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
                                struct directory **path_v,
                                       ^~~~~~~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:178:38: error: ‘struct directory’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
                               struct directory ***dpv);
                                      ^~~~~~~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:175:44: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
 RT_EXPORT extern size_t db_ls(const struct db_i *dbip,
                                            ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:203:91: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
 DEPRECATED RT_EXPORT extern int db_full_path_list_add(const char *path, int local, struct db_i *dbip, struct db_full_path_list *path_list);
                                                                                           ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:205:74: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
 DEPRECATED RT_EXPORT extern void *db_search_formplan(char **argv, struct db_i *UNUSED(dbip), struct db_search_context *);
                                                                          ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:209:20: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
             struct db_i *dbip);
                    ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:212:21: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
              struct db_i *dbip);
                     ^~~~
/home/rain/Sync/gsoc/brlcad-code/include/rt/search.h:215:49: error: ‘struct db_i’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
                                          struct db_i *dbip,
                                                 ^~~~
cc1: all warnings being treated as errors
make[3]: *** [src/libanalyze/CMakeFiles/libanalyze-obj.dir/build.make:63: src/libanalyze/CMakeFiles/libanalyze-obj.dir/api.c.o] Error 1
make: exit 2
make[2]: *** [CMakeFiles/Makefile2:18004: src/libanalyze/CMakeFiles/libanalyze-obj.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:18278: src/libged/CMakeFiles/libged.dir/rule] Error 2
make: *** [Makefile:4422: libged] Error 2

Peter Pronai (Jul 17 2018 at 19:05):

Peter Pronai (Jul 17 2018 at 19:06):

#include "common.h"
#include "bio.h"
#include "bu/hash.h"
#include "bu/list.h"
#include "bu/vls.h"
#include "dm/bview.h"
#include "rt/search.h"

starseeker (Jul 18 2018 at 11:15):

OK, so there are two ways to handle that. For simplicity, I would suggest also including the header that defines db_i (rt/ db_instance.h) in your C file along with rt/search.h

Peter Pronai (Jul 18 2018 at 15:49):

i included it in rt/search.h itself, since that's the one that seems to depend on it

starseeker (Jul 19 2018 at 02:02):

@Peter Pronai That was my initial thought, and if that works without causing additional problems it's the right thing to do. There are sufficient interdependence between the rt datatypes I wasn't sure if that would "just work", but if it does awesome.

Peter Pronai (Jul 19 2018 at 20:13):

i squashed a lot of thing into one and left out some commits and git diff tells me it's the same as the other branch

Peter Pronai (Jul 19 2018 at 20:14):

Peter Pronai (Jul 19 2018 at 20:15):

Peter Pronai (Jul 19 2018 at 22:35):

uh.... what. i can't see the squashed commits... it's the same commits as on the original branch.... what....

Peter Pronai (Jul 19 2018 at 22:36):

Peter Pronai (Jul 19 2018 at 22:37):

Peter Pronai (Jul 19 2018 at 22:38):

i can remove the unrelated fixes from it but i don't think i can separate it properly

Peter Pronai (Jul 19 2018 at 22:39):

Peter Pronai (Jul 21 2018 at 18:52):

+ b9bae437bb4cdf5fb4cf054ae761a2139f5477c8 added exec userdata pointer to struct db_plan_t
+ 01a04b0a52c9ac93f275a01961c41bdb13dd8a6c remove shorthand definitions from librt search
+ 499291ac5965258379e0cd3988d23f4fa73dcb60 basics of -exec flag for librt search
+ f277ef7afc64bdc28c24c35c01d4356854d18363 clean up memory on -exec parse error in librt
+ 9ff5cc25a6f618c957695553c79d20aa7593a9b5 librt: store result of possibly failed plan creation
+ e99b62b433c127d024ef5cddcdebb593a0aa6c89 print newline after bu_log in librt's c_exec
+ 7906e5864600773889ff771c63ca47dba2d92f8b oops. BRLCAD_OK is not 0, fixes previous commit
+ 14c64d7005b8e9fc9e3fd2669e618d54b5dd9632 remove debug logging from librt c_exec
+ 907bb124c83c9c081f42649efb6e778cc3c4f3a7 add db_search -exec related definitions
+ 40d385ad9ef257cbd5996e8719b58760d3afe492 follow naming convention in search.h
+ abfe9d6694080a2fa8a96a70bd07f21760d15889 librt/search.h: use a single argc instead of storing every argv strings's length
+ 767cd544c3d8547f4813ade9438f861cb6ae2078 small consistency fixes in c_exec
+ e5a969cdd67fd1047c6be8391b3b290ae2067e69 removed e_orig from c_exec and added a list of holes instead
+ 8856329eced85fc8b05c599da8b90fc6b6c7b980 first f_exec implementation + cleaned up c_exec a bit more
+ 2f906da457d984a567ad1e3fffdbe350bcbeb7f2 db_search -exec evaluator implemented
+ 188dd2e65fd71a97a1b154ab3129f58914b8f06d db_search: special case freeing db_plan_t if it's an -exec plan
+ 5ab4a23b53c2fba63102c67472adefa70c834af1 remove unnecessary _e_orig field from db_plan_t
+ ca05439929846dd40df2dcd739146b2d9a83b7a1 fixed some typos in db_search_free_plan
+ 9aeda60e227fc3ff1546d9e23582861058fbe7b4 make the db_search callback a const
+ e9889a108a34b472d3b7927021dacd3d525f1fdd hooked up the search related parameters in librts/search.{c,h}
+ 5af4f8ddc17da6d9ca5b6f2148a6dd397b537b31 ironed out some conflicting definitions in librt search
+ 0a7c1f6c18b7dfe57f7eeeefc4bf29be5c6bb8c0 added exec context definitions to rt/search.h
+ c19eb86b830656051d59438d811c9db56def48e9 actually, we already have NULL. removing null search context
+ 609ee66dbe3323b2e590de74b97b4fddb3d85907 use search context pointer everywhere instead of explicit params
+ 33e86bca1442ed789889333d4b942ef8bcd88a94 added NULL contexts for calls to db_search calls in libged/comb.c
+ 3c054b158de853919076abff96a83e3bb531e85c more NULL contexts for db_search calls in libged
+ 5bbc048808224fe9a4a16de3e29aaff7c3c18392 libged builds and should now support db_search contexts
+ ffe0b47290b62c89853db71313ece79d6e8adc47 added callback to MGED that evaluates Tcl with search -exec
+ fedec08c6265a0813e6e4e93224a76dce4330177 set up the callback
+ 04f8450e1848317ed9b6be753f3f97cb1934fa7d fixed existing calls to db_search
+ bba04a19849ad7c0d3a674c7a5a6a858755e0a57 improved docs for `search -exec`
+ 722b636913a42ed1d588974e8061f41e4a9f89e7 fixed doxygen
+ 66f4ce1b3bd449335f8b6159ca88da97a916e9c6 free search context after use
+ d30fdd961456142e952aa233494c516f8ca4b2be properly convert Tcl result to a boolean in MGED search callback
+ 718337152bcd6bdc8eb1b67efb2c8d341d804856 cleaned up some whitespace, probably
+ f79a9cc60d79993bf92e3a29099a3b6ff3063dc1 added db_search_context_destroy
+ 66bc37db6e1f350a5a4e2a9659a4af2d51e79d3b empty exec is a no-op that returns true
+ 0605f14e89c6b37f98822958781c63111a1a76c5 use bu_free in librt/search.c
+ 4b4e2fa52fba353d0350b647425537c260d867f4 make sure to use bu_free everywhere
+ fa2785ac87bc532a4f1da0f2f1c8bf0481dab21c fixed a bu_free call
+ 3081323a0ecf4aee443dd45c49ac07b33505e692 make the bu_free call's comment match the bu_realloc call (i thought i already committed this???)
+ 3859676ece1231779df7ac6bb59fe312ead44779 made some Tcl search examples work with -exec
+ 572f3591bd9c0f5d9f2d7ce853ade247e232a600 added a super simple and WRONG! path resolver
+ a597022aa8313cbb4c1c863c1dedd8141e4359da db_lookup works on paths too and there are name and path variants also
+ 7b0dd3cc057bff4029c850a256864396a879dedc fixed small grammatical error
+ e7233d774cdef630dd615f4d2c2b09cb66ecffaf removed lying comment
+ 442c3d883531868aa2f2ff5bd3e04e1043b0b453 i really miss rust's type system.
+ 7a6b2221d34457a190d4909b3af044653b7c6acc fixed bugs in the new db_lookup_path functionality
+ c33949237eaedf0a0125b539b7e88160ebf35394 removed trailing whitespace from db_lookup
+ 947fcb6d2328488a0d0752b1672604bf8bc787cc better and more error messages in db_lookup_path
+ 0fa85d60c05c4bef0d22f98a0bb3ab4f7b4d57ac free up the fullpath in db_lookup_path
+ 59c6b7aaa999d37894ff47e3ecd23fa01f96035e removed trailing ws from src/librt/search.c
+ caf96e4fa3bfd89629f55dc2ee1368b3a0e5c37a adhere to whitespace standards in src/librt/search.c
+ 90897eaf938b59fc55841012f954a502ffa81ac5 trim trailing ws from include/rt/search.h
+ 7719410a045e0b93b65ffa900823d9aea5dce67c trim trailing ws from src/libged/shape_recognition.cpp
+ 92ee7dcc7402276e36a7abad1be74fb9cd59ccab trim trailing ws from src/mged/cmd.c
+ 83865fc8df2f41dbe25dd659ca006c80e9964391 trim trailing ws from src/libged/search.c
+ 026346c7c31a5283a91a3c88ec056b9c6e8580d0 add missing db_instance dependency to include/rt/search.h
+ 20566236f31470e48e873cfd2e46306aac829898 removed leftover comment

Peter Pronai (Jul 21 2018 at 18:53):

Peter Pronai (Jul 21 2018 at 18:55):

now, some of the commit names are wrong because i accidentally added some files or something and noticed it too late and i'd have to do another rebase to fix that

Peter Pronai (Jul 21 2018 at 18:55):

Peter Pronai (Jul 21 2018 at 18:57):

i think i can actually skip squashing and other rebase related uglinesses and just git diff to compare commits, that way i can generate some more meaningful diffs that i can submit

starseeker (Jul 21 2018 at 21:47):

Whatever you think best - the key point is to get a set of patches in shape for committing

Sean (Jul 22 2018 at 05:08):

that is the right solution -- headers should be self-contained and make sure all the types they use are properly declared by including the headers that declare them. the only time there should ever be a problem is in the case of a bonefide cyclic dependency, which should not exist. we do not have any cycles in the current headers that I know if, so the only reason it should ever not work would likely be to a recent screw-up by someone.

Sean (Jul 22 2018 at 05:13):

@Peter Pronai note that this way of working over a git bridge is very sub-optimal and is now putting your entire project at risk because it's not committed to the main repository. please prioritize getting your patches separated and submitted cleanly. we'd rather you commit directly to the repository, not be working off on your own like this indefinitely. commit access requires at least a couple patches with no errors, conforms with style, makes some improvement (however minor), and that is mostly to demonstrate your competency in communicating change via patches that do not come with a burden.

Sean (Jul 22 2018 at 05:18):

you've clearly been very productive, but you got to work with @starseeker to get changes committed to the main svn repo. as I said on the prcolor patch months ago: "You're welcome to set up whatever you like, but submitting a 'complete' (tested and working) patch file is a fundamental developer skill that is independent of any revision control system or workflow. I think there is simply a mismatch in what you're trying to do and what is useful to share."

Sean (Jul 22 2018 at 05:20):

This is an example of something that would make for a perfect simple succinct patch that you should submit. Just the change that fixed it.

starseeker (Jul 24 2018 at 02:31):

Peter Pronai (Jul 24 2018 at 10:48):

@starseeker not really, i'm still trying to figure out how to group commits into patches :/

Sean (Jul 24 2018 at 13:15):

still?? you can always pull a single commit and submit that or cherry pick a set of commits to a branch and then diff that ... or check out an svn repo and apply your commits one at a time (git diff SHA > git.diff1 ; git diff SHA > git.diff2 ; patch -p1 < git.diff1 ; patch -p1 < git.diff2 ; svn diff > mychanges.diff) .... or even just retype a succinct change on a fresh checkout.

Sean (Jul 24 2018 at 13:16):

@Peter Pronai at a minimum, you need to be talking through what you're doing and trying here so everyone knows what's going on ...

Sean (Jul 24 2018 at 13:16):

dead silence, struggling through it on your own without talking is the worst thing you can do

Peter Pronai (Jul 24 2018 at 23:17):

i've been looking for ways to group them more or less logically but i think i should just stop overthinking it and go with whatever comes to mind first

Peter Pronai (Jul 24 2018 at 23:18):

rebasing seemed like a good way to make the commits more coherent but i ran into some errors and i have no idea how to deal with them

Peter Pronai (Jul 24 2018 at 23:19):

i've been trying to automate this more or less or at least come up with ways to make it faster, but it looks like i'll just have to do everything manually

Peter Pronai (Jul 24 2018 at 23:20):

i made a copy of the master branch (which is the same as the svn repo) and i'm making patches

Peter Pronai (Jul 24 2018 at 23:20):

also, god bless CoW file systems, i didn't have to sacrifice a bunch of extra space for cloning the repo

Peter Pronai (Jul 24 2018 at 23:36):

alternatively, i thought about just separating the combined patch by file, but that still makes for quite a large patch in most cases

Peter Pronai (Jul 24 2018 at 23:36):

Peter Pronai (Jul 24 2018 at 23:37):

Peter Pronai (Jul 24 2018 at 23:45):

Peter Pronai (Jul 24 2018 at 23:47):

Peter Pronai (Jul 24 2018 at 23:48):

....i should be writing these to my log instead of rambling on the chat, shouldn't i

Peter Pronai (Jul 25 2018 at 00:08):

Sean (Jul 25 2018 at 04:08):

rambling is perfectly fine, appreciated even ... better than only writing to your log :)

Peter Pronai (Jul 27 2018 at 00:24):

so, i'm going forward with the rebasing after all and splitting commits as needed, this should make them more manageable

Peter Pronai (Jul 27 2018 at 00:25):

after that, i can hopefully pick the changes out of order and present some nice patches

Peter Pronai (Jul 27 2018 at 00:26):

but the more realistic version is that there will be more patches that i'd like, but at least they will not include weird things

Peter Pronai (Jul 27 2018 at 00:33):

aaannnnd that's the rebase plan thingy done, i probably shouldn't run the rebase itself at 2:32 because that's just gonna lead to more problems, so i'll make a backup of the git-rebase-todo file and go dream of nicer commit histories

Sean (Jul 27 2018 at 03:33):

@Peter Pronai to be honest, this sounds entirely unacceptable. It's been an entire week and you still have no patches. You could have retyped everything on an svn checkout in a day. You've used a full week, 10% of GSoC coding period timeline, learning git and commit/patch management. Please stop.

Sean (Jul 27 2018 at 03:34):

at some point, you just have to throw in the towel doing things the automatic way, and do things manually ... I think we're way way way past that point

Sean (Jul 27 2018 at 03:45):

I suggest that unless you have all your changes ready to go today, that you stop with git and simply make one feature change on an svn checkout, make an svn patch (svn diff > change1.patch), submit that, and then get another checkout for change#2, make the changes, make the patch, submit, etc.

Sean (Jul 27 2018 at 03:46):

you can extract a patchfile for every commit you've made with: git format-patch my_branch -o my_patches

Sean (Jul 27 2018 at 03:51):

you can get an svn checkout with: svn co svn+ssh://SFUSERNAME@svn.code.sf.net/p/brlcad/code/brlcad/trunk brlcad.trunk

Sean (Jul 27 2018 at 03:51):

Peter Pronai (Jul 27 2018 at 19:06):

Peter Pronai (Jul 27 2018 at 19:07):

Peter Pronai (Jul 27 2018 at 19:08):

Peter Pronai (Jul 27 2018 at 19:14):

starseeker (Jul 28 2018 at 01:01):

@Peter Pronai This isn't quite what we were looking for - rather than the various commits in git, we were looking for "feature" patches that provided compact, isolated improvements in capabilities.

starseeker (Jul 28 2018 at 01:03):

@Peter Pronai I'm going to take this patch set and get these changes committed, so we can move forward - watch the commits. I'll post here once I've completed merging them.

starseeker (Jul 28 2018 at 03:14):

starseeker (Jul 28 2018 at 03:18):

I saw a couple of things running that first example in the man page (or rather, the tweaked version from r71286 that produces a smaller output list with the m35.g example):

1. The exec version seems to be printing carriage returns, which it probably shouldn't if it's not printing the output explicitly in addition to the -exec (not sure what find does, but I would have expected it to either exec and not print or exec and print the search result list, not do the blank lines...)

2. When doing the full path version of the search -exec (e.g. supplying "/all.g" rather than "all.g") the draw command doesn't seem to be operating on the full paths - the "who" command, when run after the search -exec with "/all.g" should indicate it drew the full paths (this can matter if there are matricies along the full path.)

starseeker (Jul 28 2018 at 03:31):

/all.g/component/bed/r908/r914/r906/r916
/all.g/component/bed/r908/r914/r906
/all.g/component/bed/r914/r906/r916
/all.g/component/frame/r532/r506/r567

mged> Z
mged> search /all.g -type region -and ( -below -bool - -or -below -bool + ) -exec draw "{}" ";"

I would expect to see either nothing or the same output as above (probably the former, unless I explicitly -print as well as -exec) but instead I see:

The good news is does draw the objects, but if I run the "who" command - which lists what is being drawn:

That list show match the full path list above, since we did a full path search - if you explicitly draw the full paths and then do a who, you'll see draw can list them properly:

mged> Z
mged> draw /all.g/component/bed/r908/r914/r906/r916 /all.g/component/bed/r908/r914/r906 /all.g/component/bed/r914/r906/r916 /all.g/component/frame/r532/r506/r567
mged> who
/all.g/component/bed/r908/r914/r906/r916 /all.g/component/bed/r908/r914/r906 /all.g/component/bed/r914/r906/r916 /all.g/component/frame/r532/r506/r567

So it looks like somewhere in the exec pipeline we need to propagate awareness of which type of search we did (full path vs unique name) and adjust what is fed into -exec accordingly

starseeker (Jul 28 2018 at 03:32):

starseeker (Jul 28 2018 at 03:36):

@Peter Pronai So from the current state, I suggest you try to make a focused patch that has -exec work on either the unique object list or the full path list depending on the search type. For -exec on full paths you'll probably want to use db_path_to_string to take the full path list and generate strings suitable for argv command execution.

Also, as a separate patch, I would see if you can eliminate the 'expected boolean value but got ""' error message

Peter Pronai (Jul 29 2018 at 02:37):

@starseeker in later versions of my branch i already use db_path_to_string, so that should be easy, but i'm not sure if the output actually should match the input at all times

Peter Pronai (Jul 29 2018 at 02:38):

Peter Pronai (Jul 29 2018 at 02:39):

ignoring bind mount shenanigans, there is only one path to a file and so that path uniquely identifies the file

Peter Pronai (Jul 29 2018 at 02:39):

Peter Pronai (Jul 29 2018 at 02:41):

an object is uniquely identified by its name and names are actually in a flat namespace. objects don't contain other objects, not really, they kind of just use them

Peter Pronai (Jul 29 2018 at 02:42):

Peter Pronai (Jul 29 2018 at 02:43):

Peter Pronai (Jul 29 2018 at 02:44):

Peter Pronai (Jul 29 2018 at 02:45):

find(1) works with commands. commands have exit codes. Tcl commands don't have exit codes.

Peter Pronai (Jul 29 2018 at 02:45):

Peter Pronai (Jul 29 2018 at 02:48):

so, I turned to the only thing it has: text output.
-exec is supposed to be a filter, if Tcl had types, -exec would take a function that returns bools. but! printing is also the side-effect of some commands! now, how should -exec know if you are only running something for its side-effects?

Peter Pronai (Jul 29 2018 at 02:48):

the easy answer for me was that it simply shouldn't, the user is expected to work their Tcl magic to give search -exec the output it expects

Peter Pronai (Jul 29 2018 at 02:51):

so, I think the nicer way to solve the noisy output problem would be to just let the user indicate that "hey, i'm running this purely for the side-effects, you can shut up about the format" and give them an equivalent to &>/dev/null

starseeker (Jul 29 2018 at 03:13):

@Peter Pronai So maybe the more straightforward problem at first is to get exec working on the full path results, when search is in full path mode

starseeker (Jul 29 2018 at 03:16):

@Peter Pronai could we at least have exec not print empty lines if the Tcl command doesn't produce any non-empty textual output? Or is that completely out of the control of exec?

Peter Pronai (Jul 29 2018 at 12:44):

Peter Pronai (Jul 29 2018 at 15:04):

hmmm...... from a Tcl scripting perspective, is it bad if we swallow empty lines? i think that could be misleading for scripts...

Peter Pronai (Jul 29 2018 at 15:06):

the current output isn't necessarily pretty, but it doesn't cause any surprises. I think that's better than being pretty??

Peter Pronai (Jul 29 2018 at 15:07):

idk, i've written some code to filter newlines out but i'm not sure how this affects scripting since I'm not familiar enough with Tcl idioms

starseeker (Jul 29 2018 at 23:58):

We can try some tests, but I'd say first let's get exec operating on full path results when the search is a full path search

Peter Pronai (Jul 30 2018 at 21:43):

ok, but I think it'd be better if it always operated on full paths because that's more uniform

Peter Pronai (Jul 30 2018 at 21:55):

Peter Pronai (Jul 30 2018 at 22:04):

Sean (Jul 31 2018 at 01:40):

Tcl commands have return codes -- they default to "TCL_OK" (aka "1") if a proc doesn't specify it or specifies "return"

Sean (Jul 31 2018 at 01:49):

I would question who/what is generating empty lines in the first place, what utility that has and whether it could change. Gut reaction is that 'search' (and by extension search -exec) shouldn't be adding/removing/filtering lines. It should only print what it's been told to print (e.g., -print) or the output from commands it ran.

Peter Pronai (Jul 31 2018 at 03:33):

I got the impression that that was just for Tcl's internal error reporting, since none of my searches on how to do bools in Tcl said "well just use the return code like in bash", instead they all talked about string tests. So does that mean I can just use Tcl_Eval's return and skip the boolean conversion?

Peter Pronai (Jul 31 2018 at 03:34):

also I imagine it being used more like a filter function? since this is embedded scripting I can't help but apply my Lua mindset to it.

Peter Pronai (Jul 31 2018 at 03:35):

Peter Pronai (Jul 31 2018 at 03:36):

Sean (Jul 31 2018 at 03:39):

an unpathed "object" is (typically) synonymous with "/object" ... just that many commands take only one or the other depending on the command's purpose

Peter Pronai (Jul 31 2018 at 03:42):

hmm, is there any command that behaves differently when given a path? i can't remember any off the bat...

Sean (Jul 31 2018 at 03:56):

Return codes are somewhat an orthogonal topic to bools. In Tcl, "everything is a string" -- https://wiki.tcl.tk/3018

Sean (Jul 31 2018 at 03:59):

Let's not get too distracted from the practical reality -- you're really referring to Tcl's C API (which is conceptually quite different from Tcl's Tcl API). For that, "man Tcl_Eval" and you'll see that you can indeed rely on a getting a whole host of potential return codes, each with a particular meaning.

Sean (Jul 31 2018 at 04:01):

again, it matters if we're talking about Tcl or C here -- but you can return a value from either (on the Tcl side, see "man n return" and "man n catch")

Peter Pronai (Jul 31 2018 at 04:14):

Peter Pronai (Jul 31 2018 at 04:16):

oh I remember why i thought bool strings were the offical way, because boolean expressions "return" a 0 or 1 string

Peter Pronai (Jul 31 2018 at 04:16):

Peter Pronai (Jul 31 2018 at 04:18):

Peter Pronai (Jul 31 2018 at 04:19):

if {1 > 0} {echo a} works but of course 1 > 0 doesn't mean anything on its own....

Peter Pronai (Jul 31 2018 at 04:22):

so, Tcl has integer returns but it doesn't use them like shells and there is no $? or $status, just catch....

Peter Pronai (Aug 01 2018 at 02:49):

ah Tcl. fun thing I found out: search -exec echo '{}' ";" (yes, possibly wrong quoting) in m35.g basically freezes MGED
i mean, it proooobably finishes after a while, but unlike find(1) which streams output nicely, Tcl has to collect everything in the result string (or object, but whatever), so it can't stream output.

Peter Pronai (Aug 01 2018 at 02:51):

not an earth-shattering revelation, yeah, but a nice example of why constructs like UNIX pipes are useful

Sean (Aug 01 2018 at 04:03):

Not really, just a completely different approach with tradeoff considerations. If all languages behaved the same, there wouldn't be much point to having different ones. :)

In the case of Tcl, their method fully separates the notion of return codes from output at a scripting level. That's particularly novel in comparison with posix/bash/ksh scripting where the two concepts are fully intermingled and you have to deal with it in convoluted logic or with presumptive convention. Even compared with a compiled language, Tcl's 'catch' command goes beyond catching a return code in that it can catch any error from a hierarchy of subcommands. Even C++-style exceptions can't really do that, or at least it would require some convention like using try-catch in leu of return codes -- which is typically quite a bad idea, discouraged.

Sean (Aug 01 2018 at 04:08):

the rabbit hole goes MUCH deeper. in Tcl, everything is a command or a string. there is actually no if/then/else language construct like you're used to seeing. that 'if' statement there is actually running an 'if' _command_ that takes two strings... two arbitrarily complex strings

Sean (Aug 01 2018 at 04:49):

patches welcome! you could certainly make it stream the output back over a pipe :)

starseeker (Aug 01 2018 at 11:09):

@Peter Pronai Can't you just feed the output from both types of returns to the commands?

starseeker (Aug 01 2018 at 11:11):

My original thought was that you would take the two different types of results, get the string forms (either dp->d_namep for objects or db_path_to_string for full paths) and pass that on to the exec cmd. Does that not work?

Peter Pronai (Aug 01 2018 at 14:04):

@starseeker yeah, i've written something like that, it just has to communicate which one to use

Peter Pronai (Aug 01 2018 at 14:05):

but i've run into some weirdness with the Tcl result thing (it always returns an empty string for some reason?) and i'm gonna debug that first

Peter Pronai (Aug 01 2018 at 14:10):

@Sean yeah, i get if being a command, but it feels a bit.... too special cased that it accepts expressions. it's certainly simpler than sh but i'm not sure it is simpler/better than rc
but then again, rc is not an embedded language, so they are quite different beasts

Peter Pronai (Aug 01 2018 at 14:14):

it's an odd language for me because it looks like rc but doesn't have its main strengths (pipes) but was built to fit a niche similar to Lua but it doesn't have the strengths of that either

Peter Pronai (Aug 01 2018 at 14:16):

(so no metatables, no iterators, not fast like Lua, doesn't have closures, probably no TCO, etc.)

Peter Pronai (Aug 01 2018 at 14:38):

Peter Pronai (Aug 01 2018 at 15:33):

Peter Pronai (Aug 01 2018 at 21:37):

tbh what i'm more concerned about is that only the last error is printed because the result string is overwritten after every Tcl_Eval

Sean (Aug 02 2018 at 05:38):

I don't actually agree that it's simpler than shell other than from a language theory perspective (the conceptual model). the language expressiveness is far more complicated than shell, the capability is far more expressive too

Sean (Aug 02 2018 at 05:39):

definitely an odd language, but no more so than perl or shell in my mind, just different tradeoffs/benefits/warts

Sean (Aug 02 2018 at 05:49):

I'd argue Tcl is far more capable and expressive as a embedded command interpreter with natural syntax (e.g., "do this" not "do(&this)" or "this.do()" or what have you)

Sean (Aug 02 2018 at 05:50):

Sean (Aug 02 2018 at 05:54):

I digress .. :) the result string getting smashed by subsequent calls happens in libged too, but how is that an issue for -exec? you're invoking each one so couldn't it capture the result? it's supposed to be returned as a true/false predicate for subsequent search command plan statements

Sean (Aug 02 2018 at 05:55):

agua:~ morrison$ find test.cpp -exec true \; -exec echo true \; -exec false \; -exec echo false \;
true

Sean (Aug 02 2018 at 05:56):

it ran all three exec's except the last one because of the 'false' command's return code

starseeker (Aug 03 2018 at 02:51):

Peter Pronai (Aug 03 2018 at 03:31):

@starseeker i made a small fix so far and have been trying to figure out how to get results out of -exec nicely and how to pass callbacks nicely, but it's. eh. it's like... i get that the output is ugly but i don't know how to make it better???

Peter Pronai (Aug 03 2018 at 03:31):

i wanted to write more stuff for that fix but i couldn't figure out how to improve it

Peter Pronai (Aug 03 2018 at 03:31):

Peter Pronai (Aug 03 2018 at 03:32):

i gotta do some bureaucracy stuff for next semester so i'll be running around the city today

Peter Pronai (Aug 03 2018 at 03:35):

the paths vs names thing is still...... well idk if it's a good idea to do so much conversion based on the input

Peter Pronai (Aug 03 2018 at 03:35):

Peter Pronai (Aug 03 2018 at 03:37):

soooo, either they should always get full paths or they should get to choose between two -exec flags or there should be two kinds of "holes"

Peter Pronai (Aug 03 2018 at 03:37):

these are also easier to implement than trying to pass down info about what the input was into c_exec

Peter Pronai (Aug 03 2018 at 03:38):

also, idk about how Tcl is usually used but could MGED have tools for higher-order functions and lambdas?

Peter Pronai (Aug 03 2018 at 03:38):

Tcl seems to have packages for that stuff but a quick test showed that the lambda package isn't in MGED

Sean (Aug 03 2018 at 03:51):

they could, but this totally violates expectations and desired behavior. if I specify "search . -exec echo {}" vs "search / -exec echo {}", they imply a different result.

Sean (Aug 03 2018 at 03:58):

maybe easier to implement but more complicated for users and error prone == not good

Peter Pronai (Aug 03 2018 at 12:18):

hmmm. well, what if I add the alternative hole syntax that forces filling with either the name or the full path but also modify the normal hole handling to use the input to decide how to fill it in?

Peter Pronai (Aug 03 2018 at 12:19):

Sean (Aug 03 2018 at 17:15):

Sean (Aug 03 2018 at 17:16):

if you're pushing the work onto the user (i.e., making it more complicated and error-prone to use), then that is a complete non-starter

Sean (Aug 03 2018 at 17:16):

Sean (Aug 03 2018 at 17:23):

that would still be unfriendly, error-prone, unusual. can you recap what the problem is? getting basic substitution working doesn't seem like it should be that complicated to me -- the user already specified whether they want absolute or relative paths or a flat unique 'basename' listing

Sean (Aug 03 2018 at 17:24):

you just need to get that information to the code doing the exec or substitution

starseeker (Aug 05 2018 at 12:13):

@Peter Pronai I've applied your 502 and 503 patches. Aside from the minor whitespace bit, they applied cleanly

starseeker (Aug 05 2018 at 12:18):

@Peter Pronai I would really recommend trying the following, if you haven't done so already:

In f_exec, instead of directly pulling the dp pointer from db_node->path, check dp_node->path->fp_len

If fp_len > 1, use db_path_to_string to make a path to feed to the subsequent logic. else, use the object d_namep.

Also, what's the bu_fnmatch on search.c:1263 trying to achieve? We don't seem to be doing anything with that result, and it's not immediately clear to me what you're trying to match the name against?

Peter Pronai (Aug 06 2018 at 23:35):

tbqh i had no idea at the time how to use the db_node as a short name so i just copy-pasted code from some simpler filter. later i rolled that change back and went back to using full path strings everywhere, but either i rolled that back too or Sean only applied some of the patches and they still included that incomplete code

Peter Pronai (Aug 06 2018 at 23:36):

tldr: i had no idea at the time what the thing i copied did so i left everything intact until i could see that it works at which point i would have eliminated unnecessary parts

Peter Pronai (Aug 07 2018 at 00:00):

starseeker (Aug 07 2018 at 02:16):

starseeker (Aug 08 2018 at 03:00):

starseeker (Aug 08 2018 at 03:08):

@Peter Pronai it may be that some of the commands can't handle full paths properly, but that is the command's problem not that of search -exec

starseeker (Aug 08 2018 at 11:17):

@Peter Pronai Please try the approach I outlined above and see what happens - even if it has problems, that patch should be a step in the right direction.

starseeker (Aug 08 2018 at 11:20):

(Reminder)
In f_exec, instead of directly pulling the dp pointer from db_node->path, check dp_node->path->fp_len
If fp_len > 1, use db_path_to_string to make a path to feed to the subsequent logic. else, use the object d_namep.

Peter Pronai (Aug 08 2018 at 20:17):

Peter Pronai (Aug 08 2018 at 20:18):

starseeker (Aug 09 2018 at 02:09):

Got most of them applied - trying to test #507, but hitting some other problem that doesn't seem to be related to that patch

starseeker (Aug 09 2018 at 02:19):

starseeker (Aug 09 2018 at 02:42):

Ah, I missed this in r71461. You actually don't want to free the strings, since this level of memory management is not the level controlling the string pointers. Looks like we also want to make sure the splan entries are allocated before freeing. Fixed in r71464

starseeker (Aug 09 2018 at 02:42):

Sean (Aug 09 2018 at 08:24):

@Peter Pronai per your log, Tcl can do lambdas but I don't think you have enough Tcl experience or time to learn the nuances of using them. Moreover, I really can't envision what you possibly need them for that wouldn't be better or more simply implemented a different way. rather, the Tcl side should frankly be as lean as possible. this gets back to one of the earliest discussions I recall @starseeker and I having with you when the project got started, about whether to provide a hook back to Tcl or bind into libged. we still may need to go the latter route just from a consistency perspective (e.g., consider python scripts running search -exec in an embedded ged context), but there is undeniable appeal in being able to call arbitrary Tcl procs during exec. Regardless, lambdas are syntactic sugar -- if you have something begging for it, I'd like to see the specific case after evals are due. Before then, seems like a complete distraction/crutch/hammer/excuse.

Sean (Aug 09 2018 at 18:40):

Major kudos on the latest patch, @Peter Pronai! Core functionality is looking nearly ready. How are docs looking? What’s next?

Peter Pronai (Aug 09 2018 at 18:43):

I was doing bugfixes but ye, good reminder, I should update the docs regarding the specific format of paths.

Peter Pronai (Aug 09 2018 at 18:44):

I wanted to use lambdas because that's kinda what I'm used to, eval with string substitution is way uglier than passing in an anonymous function

Peter Pronai (Aug 09 2018 at 18:46):

eg. one of the examples I rewrote to use -exec instead of storing everything in an array does work but tbqh i was not sure how or why

Peter Pronai (Aug 09 2018 at 18:46):

basically, lambdas are nice for one-liners where you wanna do all sorts of fun things with the parameters

starseeker (Aug 10 2018 at 03:09):

@Peter Pronai I think r71475 might address the confusion we both had about returning the results - I had the mistaken impression the array of fullpath results was trimmed down to a single db (which is not the case, and meant that once I tried a more hierarchy rich model I saw the problem.) Fortunately, we do have the necessary information available in db_node->flags.

starseeker (Aug 10 2018 at 03:10):

So it's the same solution, just keying off of the flag rather than the path length

Peter Pronai (Aug 10 2018 at 03:12):

starseeker (Aug 10 2018 at 03:12):

I was getting some odd failures to print output - can you give me your opinion on commit r71476?

starseeker (Aug 10 2018 at 03:14):

its the same logic you had in your #507 patch, but rather than checking the db_full_path length (which at this stage of the pipeline still has the full paths of all results, which was a conceptual mistake I made earlier) we can key off of the db_node->flags DB_SEARCH_RETURN_UNIQ_DP setting.

Peter Pronai (Aug 10 2018 at 03:14):

Peter Pronai (Aug 10 2018 at 03:15):

r71476 looks cleaner actually.... but afaik the result in Tcl is used for error handling not boolean returns?

Peter Pronai (Aug 10 2018 at 03:15):

starseeker (Aug 10 2018 at 03:16):

I was getting problems with the search / -exec echo "{}" ";" output, and ended up tweaking how the returns are handled. What I'm not sure of is if I broke something else in doing so - I get the behavior I expect, but I'd like you to test it out and see if it's not doing what you expect.

Peter Pronai (Aug 10 2018 at 03:18):

$ echo 'search / -exec echo "{}" ";" ' | ./bin/mged ../../db/NIST_MBE_PMI_7-10.g
/NIST_MBE_PMI_7-10.3dm
/NIST_MBE_PMI_7-10.3dm/Default
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4/Solid1_4.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3/Solid1_3.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2/Solid1_2.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1
/NIST_MBE_PMI_7-10.3dm/Default/Solid1/Solid1.s

starseeker (Aug 10 2018 at 03:21):

starseeker (Aug 10 2018 at 03:23):

for example, what's the behavior if you have one exec that does a mv {} {}.1 and then trys to exec a draw {} - will it do left-to-right ordering on execution? e.g. will the draws fail because {} is now at {}.1 thanks to the mv?

starseeker (Aug 10 2018 at 03:23):

I'd suggest determining what the behavior is and documenting it in the search man page so users know what to expect.

Peter Pronai (Aug 10 2018 at 03:23):

$ echo 'search / -exec echo "{}" ";" -exec echo foo "{}" bar ";"' | ./bin/mged ../../db/NIST_MBE_PMI_7-10.g/NIST_MBE_PMI_7-10.3dm
/NIST_MBE_PMI_7-10.3dm/Default
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4/Solid1_4.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3/Solid1_3.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2/Solid1_2.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1
/NIST_MBE_PMI_7-10.3dm/Default/Solid1/Solid1.s

starseeker (Aug 10 2018 at 03:24):

Peter Pronai (Aug 10 2018 at 03:25):

starseeker (Aug 10 2018 at 03:25):

starseeker (Aug 10 2018 at 03:26):

Peter Pronai (Aug 10 2018 at 03:27):

mged> proc foo {x} {

? echo foo $x

? echo bar $x

? }

mged> foo a
bar a

mged>

Peter Pronai (Aug 10 2018 at 03:28):

Peter Pronai (Aug 10 2018 at 03:29):

starseeker (Aug 10 2018 at 03:29):

What does that do? For that matter, what does find do with that on the OS command line?

Peter Pronai (Aug 10 2018 at 03:30):

mged> search / -exec echo "{}" ";" -exec echo "{}" ";"
/NIST_MBE_PMI_7-10.3dm
/NIST_MBE_PMI_7-10.3dm/Default
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_4/Solid1_4.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_3/Solid1_3.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2
/NIST_MBE_PMI_7-10.3dm/Default/Solid1_2/Solid1_2.s
/NIST_MBE_PMI_7-10.3dm/Default/Solid1
/NIST_MBE_PMI_7-10.3dm/Default/Solid1/Solid1.s

starseeker (Aug 10 2018 at 03:31):

@Peter Pronai So, should it be reporting the results twice? That would have been my first guess, but I'd also suggest checking what find does in a similar case.

Peter Pronai (Aug 10 2018 at 03:31):

.
.
./mozilla_rain0
./mozilla_rain0
./tmp.RZEtmTN5Tu
./tmp.RZEtmTN5Tu
./tmp.RZEtmTN5Tu/acme
./tmp.RZEtmTN5Tu/acme
./tmp.RZEtmTN5Tu/plumb
./tmp.RZEtmTN5Tu/plumb
./tmp.EzCqEyABTc
./tmp.EzCqEyABTc
./tmp.EzCqEyABTc/acme
./tmp.EzCqEyABTc/acme
./tmp.EzCqEyABTc/plumb
./tmp.EzCqEyABTc/plumb
./Temp-57bdf871-5e70-47a5-8180-755dda4eb98b
./Temp-57bdf871-5e70-47a5-8180-755dda4eb98b
./qipc_sharedmemory_qtox45bdc9c0c458c8a25fc32de3a5f6d795f60b53f9
./qipc_sharedmemory_qtox45bdc9c0c458c8a25fc32de3a5f6d795f60b53f9
./qipc_systemsem_qtox45bdc9c0c458c8a25fc32de3a5f6d795f60b53f9
./qipc_systemsem_qtox45bdc9c0c458c8a25fc32de3a5f6d795f60b53f9
./.esd-1000
./.esd-1000
./.esd-1000/socket
./.esd-1000/socket
./keepassxc-rain.socket
./keepassxc-rain.socket
./keepassxc-rain.lock
./keepassxc-rain.lock
./ns.rain.:0
./ns.rain.:0
./ns.rain.:0/plumb
./ns.rain.:0/plumb
./.X0-lock
./.X0-lock
./systemd-private-755da5f6ec004e17a9c73eeadc3d9ee7-chronyd.service-grVvTU
./systemd-private-755da5f6ec004e17a9c73eeadc3d9ee7-chronyd.service-grVvTU
./.Test-unix
./.Test-unix
./.font-unix
./.font-unix
./.XIM-unix
./.XIM-unix
./.ICE-unix
./.ICE-unix
./.X11-unix
./.X11-unix
./.X11-unix/X0
./.X11-unix/X0

starseeker (Aug 10 2018 at 03:31):

@Peter Pronai I've got to sign off for the evening - go ahead and dig into that and see what you can find (remember to discuss what you are finding on your log)

Peter Pronai (Aug 10 2018 at 03:32):

Peter Pronai (Aug 10 2018 at 03:34):

Sean (Aug 10 2018 at 05:24):

Sean (Aug 10 2018 at 05:39):

very familiar with lambdas. :) very old programming construct; quite common with functional programming languages like ML and Lisp, and more recently getting surged attention in many languages with C/C++11 getting them.

Sean (Aug 10 2018 at 05:40):

Imho, there's very few instances outside of functional languages where they do more than save you (literally) 2-4 lines of code. Not that I don't appreciate the brevity, but there are many languages where that's not the a good fit with the processing model. It's like how it's awkward to write procedural code in Java or functional code in C++ .. you can do it but it's usually just bad practice and unmaintainable to anyone other than the original author.

Sean (Aug 10 2018 at 05:46):

tcl actually embraces lambda's very strongly (heck it's -everything-is-a-command- perspective means even basic language constructs like "if [] {}" is essentially a higher-order function being given two anonymous functions with automatic scope import/export), but it's atypical in user code. there are simply easier and more readable alternatives given the procedural command environment.

Sean (Aug 10 2018 at 05:50):

FYI, this should be equiv: $ ./bin/mged -c ../../db/NIST_MBE_PMI_7-10.g search / -exec echo "{}" ";"

Sean (Aug 10 2018 at 06:16):

That is mged's fault, not Tcls. Remember that your running a highly modified embedded interpreter there. It's doing a lot of things automatically that Tcl doesn't usually do, and some of those things complicate... for example, here's what you wrote in various forms all the way up to the right form:

agua:~ morrison$ tclsh
% proc foo {x} {
echo foo $x
echo bar $x
}
% foo a
invalid command name "echo"
% proc foo {x} {
puts foo $x
puts bar $x
}
% foo a
can not find channel named "foo"
% proc foo {x} {
puts "foo $x"
puts "bar $x"
}
% foo a
foo a
bar a

Sean (Aug 10 2018 at 06:19):

In your example, 'echo' is an mged command, not a tcl command, and either it is wiping out the result or mged is running both, but resetting/overwriting as it iterates though the evaluation and gets results back from tcl.

Sean (Aug 10 2018 at 06:37):

it should, but mged may be getting in the way ... either mged, search, or the command (echo here) is probably wiping out the result string after each exec. as it also happens for me with puts, that implies it's either mged or search wiping out the result. another test with

echo "search . -exec puts \"{}\" \";\" -exec puts \"{}\" \";\" ; puts hello" | bin/mged test.g

implies that search is wiping out the result as it prints the hello along with the puts'

starseeker (Aug 10 2018 at 10:42):

@Peter Pronai Definitely you want to try and figure out what's happening in the search code then - we definitely want the multi-echo case to work.

starseeker (Aug 10 2018 at 10:58):

@Peter Pronai one data point I can offer - a quick check in gdb working with the share/db/moss.g example and the following search input:

indicates that c_exec does seem to be getting called twice, but f_exec is only being called once - e.g. it's allocating two searchplan objects that are N_EXEC types, but only getting to one of them for execution.

starseeker (Aug 10 2018 at 11:01):

I wonder if one problem we might be having here is if the search plan execution is interpreting the f_exec output as a "non-match" filter and halting filter processing once the first -exec "filter" is complete.

starseeker (Aug 10 2018 at 11:02):

If that's the case, one option would be to make sure the f_exec "filter" always returns a match.

starseeker (Aug 10 2018 at 11:03):

starseeker (Aug 10 2018 at 11:05):

mged> search / -type arb8 -exec echo "{}" ";" -name box.s
/all.g/box.r/box.s
/all.g/platform.r/platform.s

starseeker (Aug 10 2018 at 11:06):

starseeker (Aug 10 2018 at 11:08):

One possibility is to store the exec plans separately and only execute them after all the other filters have been processed, but that's not the way to go if the expected find behavior is to only apply the filters defined before each exec (i.e. if we tack another echo onto the last example above, would the first echo print both box.s and platform.s and the second print just box.s ?)

starseeker (Aug 10 2018 at 11:08):

Peter Pronai (Aug 10 2018 at 16:38):

Peter Pronai (Aug 10 2018 at 16:40):

@starseeker it prints the same thing, since "echo" always returns true (well, it should at least, there is probably some weird case like a broken pipe error where it could return false)

Peter Pronai (Aug 10 2018 at 18:17):

Peter Pronai (Aug 10 2018 at 19:18):

uh.... i removed the result string initialization from the echo command but it still doesn't work???

Peter Pronai (Aug 10 2018 at 19:21):

Peter Pronai (Aug 11 2018 at 01:11):

starseeker (Aug 11 2018 at 01:35):

starseeker (Aug 11 2018 at 01:37):

find . -name testfile.txt -exec echo {} \; -exec echo {} \;
./testfile.txt
./testfile.txt

starseeker (Aug 11 2018 at 01:39):

starseeker (Aug 11 2018 at 01:49):

starseeker (Aug 11 2018 at 01:57):

starseeker (Aug 11 2018 at 02:04):

So with that, we have search exec echo printing output, plus the "normal" search reporting. That doesn't seem to match the find behavior:

It looks like -exec may be regarded as a "printing filter" by find, which results in disabling the default output.

starseeker (Aug 11 2018 at 02:09):

nevermind, I see it - c_exec wasn't setting the db_search_isoutput flag. Trivial fix, committed.

starseeker (Aug 11 2018 at 02:19):

starseeker (Aug 11 2018 at 02:21):

Peter Pronai (Aug 11 2018 at 02:24):

starseeker (Aug 11 2018 at 02:25):

Peter Pronai (Aug 11 2018 at 02:25):

starseeker (Aug 11 2018 at 02:30):

The last quirk I'm seeing is a consequence of my use of bu_log in mged_db_search_callback - I can't capture the results of search -exec into a Tcl variable, the way I can with a normal search.

starseeker (Aug 11 2018 at 02:37):

starseeker (Aug 11 2018 at 02:45):

I think to avoid having to use bu_log, we'd need to instead somehow build up an accumulated buffer of the result strings and then return them all at once at the end of the search command run.

starseeker (Aug 11 2018 at 02:47):

in MGED, bu_log is literally putting text lines in the output window, and not accumulating them to return as part of the Tcl command result.

Peter Pronai (Aug 11 2018 at 02:49):

a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
a
b
double free or corruption (fasttop)
8982: signal: sys: abort (core dumped)
echo: exit 1

Peter Pronai (Aug 11 2018 at 02:49):

starseeker (Aug 11 2018 at 02:50):

Peter Pronai (Aug 11 2018 at 02:52):

0x00007f713adf5014 in bu_free (ptr=0x55d3bd9ab250, str=0x7f713e93740e "e_argv") at /home/rain/Sync/gsoc/brlcad-code/src/libbu/malloc.c:200

Peter Pronai (Aug 11 2018 at 02:52):

starseeker (Aug 11 2018 at 02:53):

/me ponders... should the results of search -exec be storable in a Tcl variable as a result of running search? we might be able to do it with our own bu_log hook in the search callback...

starseeker (Aug 11 2018 at 02:53):

starseeker (Aug 11 2018 at 02:54):

Peter Pronai (Aug 11 2018 at 02:54):

starseeker (Aug 11 2018 at 02:54):

Peter Pronai (Aug 11 2018 at 02:55):

starseeker (Aug 11 2018 at 02:55):

Peter Pronai (Aug 11 2018 at 02:56):

hey, this isn't the new and improved node freeer thingy i wrote that fixed my old and horrible node freeer thingy...

Peter Pronai (Aug 11 2018 at 02:56):

starseeker (Aug 11 2018 at 02:57):

I might have made a mistake, but the patch I applied looked like it was freeing too much...

starseeker (Aug 11 2018 at 02:57):

Peter Pronai (Aug 11 2018 at 02:59):

Peter Pronai (Aug 11 2018 at 03:00):

starseeker (Aug 11 2018 at 03:00):

Peter Pronai (Aug 11 2018 at 03:00):

starseeker (Aug 11 2018 at 03:00):

Peter Pronai (Aug 11 2018 at 03:01):

Peter Pronai (Aug 11 2018 at 03:02):

starseeker (Aug 11 2018 at 03:03):

starseeker (Aug 11 2018 at 03:04):

Let me back up and use your original freeer - if I missed the bu_strdupm I probably introduced a memory leak

Peter Pronai (Aug 11 2018 at 03:04):

starseeker (Aug 11 2018 at 03:11):

r71487 is close to your earlier patch, but if I add in the bu_free(splan->p_un.ex._e_argv[i], "e_argv[i]"); loop I get memory corruption. Did I strip out the bu_strdupm by mistake?

starseeker (Aug 11 2018 at 03:13):

No, it's still there... why do I get a crash if I free it when I free the plans??

Peter Pronai (Aug 11 2018 at 03:13):

starseeker (Aug 11 2018 at 03:14):

starseeker (Aug 11 2018 at 03:17):

r71488 works for me, but if I uncomment the loop freeing the e_argv entries I get a SIGSEGV

starseeker (Aug 11 2018 at 03:18):

Peter Pronai (Aug 11 2018 at 03:27):

Peter Pronai (Aug 11 2018 at 03:29):

starseeker (Aug 11 2018 at 03:29):

starseeker (Aug 11 2018 at 03:30):

starseeker (Aug 11 2018 at 03:32):

@Peter Pronai at this point, I would double check that all the man page examples work.

Peter Pronai (Aug 11 2018 at 03:34):

starseeker (Aug 11 2018 at 03:36):

The only other thing I can see that is missing is the possibility to capture the output of the execs into a Tcl variable, and that would probably require some pretty fancy voodoo at the libged search.c level with bu_log hooks - db_search is not set up to return arbitrary string output (which is not only possible but likely with arbitrary exec calls) so it would have to be handled at the libged level.

starseeker (Aug 11 2018 at 03:37):

I would also add a man page example illustrating the behavior of multiple -exec calls - what to expect in terms of behavior.

starseeker (Aug 11 2018 at 03:38):

you can use the moss.g examples from this chat log as a starting point - examples where I was wondering what the behavior would be (and what the right behavior should be) could be used to guide a man page discussion of the behavior of multiple -exec calls.

Peter Pronai (Aug 11 2018 at 03:42):

starseeker (Aug 11 2018 at 03:43):

one example might be if you have one -exec call that is a mv and a subsequent list call that is expecting the original name:

I would expect all of the "l" list commands to fail, since the mv will be done first. On the other hand, I would expect

starseeker (Aug 11 2018 at 03:45):

@Peter Pronai the complication with storing the results in Tcl is that we're Tcl_Evaling the commands from exec in the interpreter, so accumulating the results in Tcl gets tricky. There's probably a way to make it work, but I'm not exactly sure how to do it.

You can certainly try to figure it out, but before doing that I would add the man page discussion, and make sure you are ready to submit what you need to submit to Google for the wrap-up.

Peter Pronai (Aug 11 2018 at 03:48):

Peter Pronai (Aug 11 2018 at 03:50):

starseeker (Aug 11 2018 at 03:50):

Am I correct that supporting that would require us to explicitly store the spaces when you're splitting up the -exec input strings?

Peter Pronai (Aug 11 2018 at 03:51):

starseeker (Aug 11 2018 at 03:51):

Peter Pronai (Aug 11 2018 at 03:52):

starseeker (Aug 11 2018 at 03:52):

Peter Pronai (Aug 11 2018 at 03:52):

Peter Pronai (Aug 11 2018 at 03:53):

starseeker (Aug 11 2018 at 03:53):

Peter Pronai (Aug 11 2018 at 03:54):

starseeker (Aug 11 2018 at 03:54):

Peter Pronai (Aug 11 2018 at 03:54):

starseeker (Aug 11 2018 at 03:55):

this is awesome stuff - we've been wanting an -exec option in search for literally years

Peter Pronai (Aug 11 2018 at 03:57):

Peter Pronai (Aug 12 2018 at 00:29):

ok i think it finally works. wow. TIL water is wet and doing string stuff in C is prone to errors

Peter Pronai (Aug 12 2018 at 00:35):

./bin/mged ../../db/moss.g 'search -exec echo "{}AAAA{}{}B" ";" -exec echo "foo{}" "bar{}" ";"'

all.gAAAAall.gall.gB
fooall.g barall.g
light.rAAAAlight.rlight.rB
foolight.r barlight.r
LIGHTAAAALIGHTLIGHTB
fooLIGHT barLIGHT
tor.rAAAAtor.rtor.rB
footor.r bartor.r
torAAAAtortorB
footor bartor
ellipse.rAAAAellipse.rellipse.rB
fooellipse.r barellipse.r
ellipse.sAAAAellipse.sellipse.sB
fooellipse.s barellipse.s
cone.rAAAAcone.rcone.rB
foocone.r barcone.r
cone.sAAAAcone.scone.sB
foocone.s barcone.s
box.rAAAAbox.rbox.rB
foobox.r barbox.r
box.sAAAAbox.sbox.sB
foobox.s barbox.s
platform.rAAAAplatform.rplatform.rB
fooplatform.r barplatform.r
platform.sAAAAplatform.splatform.sB
fooplatform.s barplatform.s

starseeker (Aug 12 2018 at 01:32):

@Peter Pronai that patch didn't apply cleanly to trunk - did I miss a precursor patch?

Peter Pronai (Aug 12 2018 at 01:33):

Peter Pronai (Aug 12 2018 at 01:35):

Peter Pronai (Aug 12 2018 at 01:44):

starseeker (Aug 12 2018 at 01:44):

I did have one commit making sure we had a callback before trying to exec it - that might complicate the merge

Peter Pronai (Aug 12 2018 at 02:11):

np, i just reverted it then merged then reverted the revert then resolved the conflict

Peter Pronai (Aug 12 2018 at 02:12):

Peter Pronai (Aug 12 2018 at 02:19):

starseeker (Aug 12 2018 at 02:28):

starseeker (Aug 12 2018 at 02:29):

Peter Pronai (Aug 12 2018 at 02:30):

starseeker (Aug 12 2018 at 02:31):

Peter Pronai (Aug 12 2018 at 02:32):

starseeker (Aug 12 2018 at 02:36):

starseeker (Aug 12 2018 at 02:37):

starseeker (Aug 12 2018 at 02:39):

starseeker (Aug 12 2018 at 02:40):

Peter Pronai (Aug 13 2018 at 13:40):

sent the docs patch, I think that's basically it? gonna upload it now and send the eval

starseeker (Aug 16 2018 at 01:31):

@Peter Pronai looking at your README.md - what are the conflicts in some of the commits? Is this the list of patches showing the stages of work (i.e. changes and subsequent changes in other commits?)

Peter Pronai (Aug 16 2018 at 04:12):

@starseeker it's (almost) all of the commits by me, not all of them were sent as patches

Peter Pronai (Aug 16 2018 at 04:13):

Peter Pronai (Aug 16 2018 at 04:14):

come to think of it, maybe i should describe which branches are actually interesting......

Peter Pronai (Aug 18 2018 at 00:47):

search -exec eval {
    attr set {} color [expr {int(rand()*225)+30}]/[expr {int(rand()*225)+30}]/[expr {int(rand()*225)+30}]
    attr set {} shader {glass {tr 0.7 ri 1}}
} ";"

eval {
    puts a
    puts b
}

starseeker (Aug 22 2018 at 01:23):

@Peter Pronai Looking at the man page, for the search examples of "Bad" and "Good" do you think you could make a version that has one "correct" example paired with one "incorrect" example per table row? Having multiple "good" examples on the same line gives the impression that you need two exec calls instead of one to fix the one on the first row (for example.)

Stream: Google Summer of Code

Topic: search -exec

Sean (May 17 2018 at 03:25):

Peter Pronai (May 18 2018 at 15:33):

starseeker (May 19 2018 at 01:07):

starseeker (May 19 2018 at 02:28):

Peter Pronai (May 20 2018 at 16:26):

Peter Pronai (May 20 2018 at 16:27):

Peter Pronai (May 20 2018 at 16:29):

Peter Pronai (May 20 2018 at 16:30):

starseeker (May 22 2018 at 02:56):

starseeker (May 22 2018 at 02:58):

starseeker (May 22 2018 at 02:59):

Peter Pronai (May 22 2018 at 11:52):

Peter Pronai (May 22 2018 at 14:28):

Peter Pronai (May 22 2018 at 14:29):

Peter Pronai (May 22 2018 at 14:29):

Peter Pronai (May 25 2018 at 02:32):

starseeker (May 26 2018 at 02:02):

starseeker (May 27 2018 at 03:15):

starseeker (May 27 2018 at 03:19):

Peter Pronai (May 27 2018 at 03:20):

Peter Pronai (May 27 2018 at 03:21):

Peter Pronai (May 27 2018 at 03:21):

starseeker (May 27 2018 at 03:21):

Peter Pronai (May 27 2018 at 03:22):

Peter Pronai (May 27 2018 at 03:24):

starseeker (May 27 2018 at 03:24):

Peter Pronai (May 27 2018 at 03:24):

starseeker (May 27 2018 at 03:25):

Peter Pronai (May 27 2018 at 03:26):

Peter Pronai (May 27 2018 at 03:26):

starseeker (May 27 2018 at 03:26):

Peter Pronai (May 27 2018 at 03:26):

Peter Pronai (May 27 2018 at 03:27):

Peter Pronai (May 27 2018 at 03:27):

starseeker (May 27 2018 at 03:28):

starseeker (May 27 2018 at 03:28):

Peter Pronai (May 27 2018 at 03:29):

Peter Pronai (May 27 2018 at 03:29):

starseeker (May 27 2018 at 03:31):

starseeker (May 27 2018 at 03:32):

starseeker (May 27 2018 at 03:33):

Peter Pronai (May 27 2018 at 03:33):

Peter Pronai (May 27 2018 at 03:35):

starseeker (May 27 2018 at 03:35):

Peter Pronai (May 27 2018 at 03:37):

starseeker (May 27 2018 at 03:37):

starseeker (May 27 2018 at 03:38):

Peter Pronai (May 27 2018 at 03:39):

starseeker (May 27 2018 at 03:39):

starseeker (May 27 2018 at 03:39):

Peter Pronai (May 27 2018 at 03:39):

starseeker (May 27 2018 at 03:40):

Peter Pronai (May 27 2018 at 03:41):

Peter Pronai (May 27 2018 at 03:41):

Peter Pronai (May 27 2018 at 03:41):

Peter Pronai (May 27 2018 at 03:42):

Peter Pronai (May 28 2018 at 14:26):

starseeker (May 28 2018 at 14:26):

Peter Pronai (May 28 2018 at 14:28):

Peter Pronai (May 28 2018 at 14:28):

Sean (May 28 2018 at 14:30):

starseeker (May 28 2018 at 14:35):

starseeker (May 28 2018 at 14:36):

Peter Pronai (May 29 2018 at 17:55):

starseeker (May 30 2018 at 00:50):

Peter Pronai (May 30 2018 at 00:56):

starseeker (May 30 2018 at 01:48):

starseeker (May 30 2018 at 01:49):

starseeker (May 30 2018 at 01:50):

starseeker (May 30 2018 at 01:51):

starseeker (May 30 2018 at 01:56):

Peter Pronai (May 30 2018 at 02:00):

Sean (May 30 2018 at 06:12):

Sean (May 30 2018 at 06:13):

Sean (May 30 2018 at 06:16):

starseeker (Jun 02 2018 at 01:20):

Peter Pronai (Jun 02 2018 at 02:25):

Peter Pronai (Jun 02 2018 at 02:25):