kdb-ls-depth: Implement depth parameter for kdb ls #1345

e1528532 · 2017-02-15T16:32:17Z

Purpose

Like said, the purpose of this PR is to provide a depth parameter for kdb ls, like we have in kdb complete as requested in #1237

Checklist

commit messages are fine (with references to issues)
I ran all tests and everything went fine
I added unit tests
affected documentation is fixed
I added code comments, logging, and assertions
meta data is updated (e.g. README.md of plugins)

@markus2330 please review my pull request, and keep my comments in #1237 in mind.

markus2330 · 2017-02-15T16:49:02Z

@tom-wa can you check if you like the behavior please?

markus2330

Please add some shell recorder tests. (Also from basic functionality)

markus2330 · 2017-02-15T16:50:24Z

src/tools/kdb/ls.cpp

@@ -21,12 +21,9 @@ LsCommand::LsCommand () : kdb (root)
 {
 }

-int LsCommand::execute (Cmdline const & cl)
+int LsCommand::execute (const Cmdline & cl)


Please keep it <obj> const & cl, otherwise we have a mix of style.

I used const before the class type all the time, also in kdb complete. Should i change it too in there?

and what about something like const Key key (without &), should it also be Key const key?

I used const before the class type all the time, also in kdb complete. Should i change it too in there?

It is not urgent, please not within this PR. Maybe together with #1328 or even seperately.

and what about something like const Key key (without &), should it also be Key const key?

It actually does not make much sense to do that, at least not like you did it. It will increase/decrease the ref counter unnecessarily.

Also const int as formal parameter is of little use, the value is copied anyway, why protect it from writing to it?

The position of const seems to be inconsistent at some other places, e.g. const char *, maybe one of the llvm tools can change it automatically?

Btw. it is good that you use const so frequently.

Ok i have to admit i haven't thought about the ref counter of the Keys yet. You are right in that cases it probably makes more sense to pass them around by reference if applicable to avoid that.

But i meant the general cases where i pass a const object around, for example a string. Should i use const string foo or string const foo in the future?

Doesn't the const int indicate that this integer will not change in this method? I've seen it more from the perspective of immutability. If i want to express that some parameter should not change in a method, i mark it as const, regardless of wheter its getting passed by copy so modifications wouldn't make a difference outside the method.

Okay, its a very strict sense of immutability, which is a bit unusual, e.g. https://isocpp.org/wiki/faq/const-correctness does not recommend it. It is a bit misleading from the interface perspective. But I am okay with it if you use it. (For APIs (like libtools) we should check if all compilers are ABI compatible when const needs to be removed).

About the placement of const: Anything is okay as long as it is used consistently. It is not so often used in the code base, so even if llvm cannot do it, you can also change it by hand if you prefer one over the other. Otherwise simply use, what is already used.

… improve reference const style ElektraInitiative#1345

markus2330 · 2017-02-16T16:37:26Z

tests/shell/shell_recorder/kdb_ls.esr

+
+< kdb mount $File $Mountpoint $Storage
+
+< kdb set $Mountpoint/test bla


You can and should use the tutorial style syntax or did you have any problems with it?

Ahh you've already seen it, i was still messing around with the shell recorder. What do you mean with tutorial syntax? I haven't found anything about that in the Readme. Do you mean i should write a tutorial how to use kdb ls and then use this tutorial wrapper?

I'm not really sure how the shellrecorder has to be used correctly yet. I've created my esr script which contains some commands that will be done on a temporary mountpoint to avoid polluting the host system. I think the extra mount was totally unnecessary as it seems the shell recorder already takes care about that, so i removed it. Also i moved the mountpoint to user where everyone can write to without having root rights.

Then i execute this script via the shellrecorder and grab the protocol it produces (kdb_lsprotocol). After checking if the output of each command was as expected in the protocol, the test can be executed by running the shellrecorder script again and comparing the output to the stored protocol file, which should match (and did match when i execute the test directly via ./shell_recorder.sh kdb_ls.esr kdb_lsprotocol). This way we can prevent regression bugs in future versions.

But does the test execution (make_run) even compare the output against the protocol file? Inspecting the CMakeFile made me doubt that. Do i have to copy the expected output back in the script file (kdb_ls.esr) and check it everywhere with the STDOUT command? That would be much more cumbersome than simply comparing against the protocol.

And in all the .esr files it uses the kdb commands directly, but if i don't use e.g. kdb ls instead of just ls, it will fail as it actually tries to use my system's ls command and not the one of kdb.

… improve reference const style ElektraInitiative#1345

… and examples ElektraInitiative#1345

e1528532 · 2017-02-23T11:04:30Z

checked out the tutorial syntax and used the example section in the documentation of kdb-ls to implement the test cases, so that they serve as examples for users at the same time. The syntax looks not perfect, but still a win/win i think.

…nce const style #1345

tom-wa · 2017-05-09T01:34:41Z

i can't get the short options to work
the only thing thats a little bit annoying is the difference between having and not having a / at the end

kdb ls system/info/elektra --max-depth=1
kdb ls system/info/elektra/ --max-depth=1
system/info/elektra/constants
system/info/elektra/uname

…complete ElektraInitiative#1237

… and examples ElektraInitiative#1345

e1528532 · 2017-06-15T18:27:51Z

fixed the short option argument issue, depends on PR #1510 now.
that behavior with the slash at the end is intended as explained in #1237 to be consistent with how min and max depth work with kdb complete. If we want that consistency.

e1528532 · 2017-07-26T13:49:10Z

if we are fine with that, we can merge this i think

markus2330 · 2017-07-26T16:44:43Z

jenkins build fast please

markus2330 · 2017-07-26T16:47:40Z

Looks good, thank you. But seems like we have a conflict here 😢 please rebase.

sanssecours · 2017-07-26T18:01:13Z

Sorry, I should not have resolved the conflict 😳. I just wanted to try GitHub’s online editor. Anyway, as far as I can tell, rebasing should work the same as always. I already rebased the PR locally. Sorry again, if commit cc0afe6 caused any problems.

markus2330 · 2017-07-26T19:14:47Z

Seems like you merged master into this branch?

Merging now would work. If rebasing can be easily done I would prefer rebasing nevertheless.

@e1528532 Simply readd "ready to merge" if done.

sanssecours · 2017-07-26T19:27:39Z

Seems like you merged master into this branch?

Yes, I assume commit cc0afe6 is the result of the command git merge master.

If rebasing can be easily done I would prefer rebasing nevertheless.

If you want, I can just push a rebased version of this pull request.

sanssecours · 2017-07-26T19:48:21Z

src/tools/kdb/ls.cpp

+{
+	const int offset = root.getBaseName ().empty () || shallShowNextLevel (cl.arguments[0]) ? 1 : 0;
+	const int relativeMinDepth = rootDepth + cl.minDepth + offset;
+	const int relativeMaxDepth = std::max (cl.maxDepth, rootDepth + cl.maxDepth + offset);


Looks like this line may cause an integer overflow:

../src/tools/kdb/ls.cpp:76:64: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../src/tools/kdb/ls.cpp:76:64

.

… and examples #1345

e1528532 mentioned this pull request Feb 15, 2017

add depth/levels option to kdb ls #1237

Closed

markus2330 requested a review from tom-wa February 15, 2017 16:48

markus2330 suggested changes Feb 15, 2017

View reviewed changes

e1528532 added a commit to e1528532/libelektra that referenced this pull request Feb 16, 2017

kdb-complete: fix minor issues with mountpoints ElektraInitiative#1328,…

a390a8a

… improve reference const style ElektraInitiative#1345

e1528532 added a commit to e1528532/libelektra that referenced this pull request Feb 16, 2017

kdb-ls-depth: unify const & style ElektraInitiative#1345

68fb0c2

markus2330 reviewed Feb 16, 2017

View reviewed changes

e1528532 added a commit to e1528532/libelektra that referenced this pull request Feb 17, 2017

kdb-complete: fix minor issues with mountpoints ElektraInitiative#1328,…

c5b87bc

… improve reference const style ElektraInitiative#1345

e1528532 mentioned this pull request Feb 21, 2017

project diary #1087

Closed

27 tasks

e1528532 added a commit to e1528532/libelektra that referenced this pull request Feb 23, 2017

kdb-ls-depth: add tutorial shell recorder tests serving both as tests…

0fc5420

… and examples ElektraInitiative#1345

e1528532 added the work in progress label Feb 23, 2017

markus2330 pushed a commit that referenced this pull request Feb 24, 2017

kdb-complete: fix minor issues with mountpoints #1328, improve refere…

db7c8f9

…nce const style #1345

e1528532 added 6 commits June 15, 2017 19:10

kdb-ls-depth: Implement min/max depth setting to kdb ls, like in kdb …

b07f2b2

…complete ElektraInitiative#1237

kdb-ls-depth: unify const & style ElektraInitiative#1345

74c2de6

kdb-ls-depth: add shellrecorder test case

d17f59f

kdb-ls-depth: cleanup shell recorder tests

4d57aac

kdb-ls-depth: add tutorial shell recorder tests serving both as tests…

272fab6

… and examples ElektraInitiative#1345

kdb-ls-depth: remove deprecated shell recorder files

a539cdc

e1528532 force-pushed the kdb-ls-depth branch from c3aba22 to a539cdc Compare June 15, 2017 17:18

kdb-ls-depth: fix getopt string for arguments

105ad46

e1528532 mentioned this pull request Jun 15, 2017

fix-getopt #1510

Merged

6 tasks

e1528532 added ready to merge and removed work in progress labels Jul 26, 2017

markus2330 approved these changes Jul 26, 2017

View reviewed changes

Merge branch 'master' into kdb-ls-depth

cc0afe6

markus2330 removed the ready to merge label Jul 26, 2017

sanssecours reviewed Jul 26, 2017

View reviewed changes

sanssecours mentioned this pull request Jul 27, 2017

Add Depth Parameter to kdb ls #1554

Merged

2 tasks

markus2330 closed this in #1554 Jul 27, 2017

markus2330 pushed a commit that referenced this pull request Jul 27, 2017

kdb-ls-depth: unify const & style #1345

5b88b7a

markus2330 pushed a commit that referenced this pull request Jul 27, 2017

kdb-ls-depth: add tutorial shell recorder tests serving both as tests…

77f8836

… and examples #1345

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kdb-ls-depth: Implement depth parameter for kdb ls #1345

kdb-ls-depth: Implement depth parameter for kdb ls #1345

e1528532 commented Feb 15, 2017 •

edited

Loading

markus2330 commented Feb 15, 2017 •

edited

Loading

markus2330 left a comment

markus2330 Feb 15, 2017

e1528532 Feb 15, 2017 •

edited

Loading

markus2330 Feb 15, 2017

e1528532 Feb 15, 2017

markus2330 Feb 15, 2017

markus2330 Feb 16, 2017

e1528532 Feb 16, 2017 •

edited

Loading

e1528532 commented Feb 23, 2017

tom-wa commented May 9, 2017

e1528532 commented Jun 15, 2017

e1528532 commented Jul 26, 2017

markus2330 commented Jul 26, 2017

markus2330 commented Jul 26, 2017

sanssecours commented Jul 26, 2017

markus2330 commented Jul 26, 2017 •

edited

Loading

sanssecours commented Jul 26, 2017

sanssecours Jul 26, 2017


		< kdb mount $File $Mountpoint $Storage

		< kdb set $Mountpoint/test bla

kdb-ls-depth: Implement depth parameter for kdb ls #1345

kdb-ls-depth: Implement depth parameter for kdb ls #1345

Conversation

e1528532 commented Feb 15, 2017 • edited Loading

Purpose

Checklist

markus2330 commented Feb 15, 2017 • edited Loading

markus2330 left a comment

Choose a reason for hiding this comment

markus2330 Feb 15, 2017

Choose a reason for hiding this comment

e1528532 Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

markus2330 Feb 15, 2017

Choose a reason for hiding this comment

e1528532 Feb 15, 2017

Choose a reason for hiding this comment

markus2330 Feb 15, 2017

Choose a reason for hiding this comment

markus2330 Feb 16, 2017

Choose a reason for hiding this comment

e1528532 Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

e1528532 commented Feb 23, 2017

tom-wa commented May 9, 2017

e1528532 commented Jun 15, 2017

e1528532 commented Jul 26, 2017

markus2330 commented Jul 26, 2017

markus2330 commented Jul 26, 2017

sanssecours commented Jul 26, 2017

markus2330 commented Jul 26, 2017 • edited Loading

sanssecours commented Jul 26, 2017

sanssecours Jul 26, 2017

Choose a reason for hiding this comment

e1528532 commented Feb 15, 2017 •

edited

Loading

markus2330 commented Feb 15, 2017 •

edited

Loading

e1528532 Feb 15, 2017 •

edited

Loading

e1528532 Feb 16, 2017 •

edited

Loading

markus2330 commented Jul 26, 2017 •

edited

Loading