New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference plugin #2238

Open
wants to merge 12 commits into
base: master
from

Conversation

2 participants
@kodebach
Contributor

kodebach commented Sep 14, 2018

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains *(my name)*)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)
@markus2330

Looks very good. Only small elektrifications.

Show outdated Hide outdated src/plugins/reference/README.md Outdated
Show outdated Hide outdated src/plugins/reference/README.md Outdated
## Introduction
This plugin serves one single purpose: the validation of reference values inside of keys.

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

What are reference values?

@markus2330

markus2330 Sep 14, 2018

Contributor

What are reference values?

two values define to different modes of operation for the plugin.
If a key has the metakey `check/reference` with value `single`, the plugin reads the value
of the key and produces an error, if the value is not a valid reference.

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

What is a valid reference?

@markus2330

markus2330 Sep 14, 2018

Contributor

What is a valid reference?

Show outdated Hide outdated src/plugins/reference/README.md Outdated
### Construction of the reference graph
The process starts with a key `X`, which has the metakey `check/reference` set to the value

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

Which one is the key X?

@markus2330

markus2330 Sep 14, 2018

Contributor

Which one is the key X?

This comment has been minimized.

@kodebach

kodebach Sep 14, 2018

Contributor

X is the key which has check/reference set to recursive.

So in the case of kdb setmeta user/tests/somekey/ref check/reference recursive the key user/tests/somekey/ref will be X.

@kodebach

kodebach Sep 14, 2018

Contributor

X is the key which has check/reference set to recursive.

So in the case of kdb setmeta user/tests/somekey/ref check/reference recursive the key user/tests/somekey/ref will be X.

Show outdated Hide outdated src/plugins/reference/README.md Outdated
as the `refname`.
Once the whole process is finished, we will have a graph of the whole reference structure
stemming from `X`. This graph can then be check for acyclicity.

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

the same is needed for the fallback/override links. Maybe do this in the same plugin? Or in a separate plugin reusing the graph code?

@markus2330

markus2330 Sep 14, 2018

Contributor

the same is needed for the fallback/override links. Maybe do this in the same plugin? Or in a separate plugin reusing the graph code?

This comment has been minimized.

@kodebach

kodebach Sep 14, 2018

Contributor

My plan was to represent the graph as a separate KeySet. So I think it would be easiest to just copy the code to wherever fallback/override links are currently processed.

@kodebach

kodebach Sep 14, 2018

Contributor

My plan was to represent the graph as a separate KeySet. So I think it would be easiest to just copy the code to wherever fallback/override links are currently processed.

This comment has been minimized.

@markus2330

markus2330 Sep 19, 2018

Contributor

Maybe we can also add it to libmeta, it seems to fit to the topological sort. Or we move elektraSortTopology together with your function to libgraph?

@markus2330

markus2330 Sep 19, 2018

Contributor

Maybe we can also add it to libmeta, it seems to fit to the topological sort. Or we move elektraSortTopology together with your function to libgraph?

and `..` is unnecessary and discouraged. Therefore a warning will be emitted, if `.` or `..`
are encountered. The same is true if a path segment other then the first one is `.`.
If a keyname matches the globbing expression it will be considered a valid reference

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

See the spec plugin, Elektra has small differences in globbing characters.

_ matches key names (except arrays) and # matches array entries. See src/libs/elektra/keyname.c

@markus2330

markus2330 Sep 14, 2018

Contributor

See the spec plugin, Elektra has small differences in globbing characters.

_ matches key names (except arrays) and # matches array entries. See src/libs/elektra/keyname.c

This comment has been minimized.

@kodebach

kodebach Sep 14, 2018

Contributor

I will adapt the README.

Is * still allowed? Does still behave the same? If I read the spec plugin code correctly this is the case. Also if I read the code correctly (the README also agrees with me), _ is just replaced with * so it matches arrays as well.

Also: we should probably move the matching code somewhere like elektra-ease, so that everybody knows this syntax is not spec plugin specific, and so that we avoid code duplication.

@kodebach

kodebach Sep 14, 2018

Contributor

I will adapt the README.

Is * still allowed? Does still behave the same? If I read the spec plugin code correctly this is the case. Also if I read the code correctly (the README also agrees with me), _ is just replaced with * so it matches arrays as well.

Also: we should probably move the matching code somewhere like elektra-ease, so that everybody knows this syntax is not spec plugin specific, and so that we avoid code duplication.

This comment has been minimized.

@markus2330

markus2330 Sep 19, 2018

Contributor

Good question, it was rather by accident that * still works but it somehow makes sense to be not incompatible to glob(3) as people already know it (but only extend it to only match (non-)arrays).

And yes, a good idea to move the matching code. Unfortunately "fnmatch" might cause the library to be removed and there are already quite many libs depending on libease. So maybe it is better to create a libglob?

@markus2330

markus2330 Sep 19, 2018

Contributor

Good question, it was rather by accident that * still works but it somehow makes sense to be not incompatible to glob(3) as people already know it (but only extend it to only match (non-)arrays).

And yes, a good idea to move the matching code. Unfortunately "fnmatch" might cause the library to be removed and there are already quite many libs depending on libease. So maybe it is better to create a libglob?

We mount it by executing:
```sh
kdb mount alternative.ini user/tests/reference/alternative ni reference

This comment has been minimized.

@markus2330

markus2330 Sep 14, 2018

Contributor

It is better to use HERE documents, then this works as shell recorder script.

@markus2330

markus2330 Sep 14, 2018

Contributor

It is better to use HERE documents, then this works as shell recorder script.

@kodebach

This comment has been minimized.

Show comment
Hide comment
@kodebach

kodebach Oct 19, 2018

Contributor

@markus2330 please review code

The README will need some more work and some code should probably be moved to other places, but all functionality should now be implemented.

Contributor

kodebach commented Oct 19, 2018

@markus2330 please review code

The README will need some more work and some code should probably be moved to other places, but all functionality should now be implemented.

@markus2330

Thank you, looks like there are many improvements in a quick look-through!

Please finish up the PR (incl. docu) before requesting reviews or ask specific questions. It is difficult to review half-done PRs.

* @retval true if @p name is a valid array item
* @retval false otherwise
*/
bool isArrayName (const char * name) // TODO: move? definitely useful elsewhere, e.g. reference plugin, libease, etc.

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

Isn't this the same as elektraArrayValidateName from libease?

If not, putting this to libease makes more sense.

@markus2330

markus2330 Oct 20, 2018

Contributor

Isn't this the same as elektraArrayValidateName from libease?

If not, putting this to libease makes more sense.

This comment has been minimized.

@kodebach

kodebach Oct 20, 2018

Contributor

Yes, this is the basically the same as in libease, but I wasn't sure whether a dependency on libease just for this functionality was a good idea. If the libglobbing depends on libease, then I don't see the advantage in having a separate libglobbing at all.

Personally I think that this function should be part of the core, because elektraWriteArrayNumberis also declared inkdbhelper.hand implemented inkeyset.c`

@kodebach

kodebach Oct 20, 2018

Contributor

Yes, this is the basically the same as in libease, but I wasn't sure whether a dependency on libease just for this functionality was a good idea. If the libglobbing depends on libease, then I don't see the advantage in having a separate libglobbing at all.

Personally I think that this function should be part of the core, because elektraWriteArrayNumberis also declared inkdbhelper.hand implemented inkeyset.c`

#define ELEKTRA_GLOB_NOMATCH (-1)
int keyGlob (const Key * key, const char * pattern);

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

New public names should start with elektra (so it should be elektraKeyGlob)

@markus2330

markus2330 Oct 20, 2018

Contributor

New public names should start with elektra (so it should be elektraKeyGlob)

* <li> '?' matches any single character except '/' </li>
* <li> '#', when used as "/#/" (or "/#" at the end of @p pattern), matches a valid array item </li>
* <li> '_', when used as "/_/"(or "/_" at the end of @p pattern), matches a key part that is <b>not</b> a valid array item </li>
* <li> if the pattern ends with "&#47;**", matching key names may contain arbitrary suffixes </li>

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

Thank you, very nice feature! I was just thinking this would be great. Does this also makes sense within the key name?

And what about using __ so that it is clear that it is an Elektra extension?

About how it is written down here: Why using '/' instead of /?

@markus2330

markus2330 Oct 20, 2018

Contributor

Thank you, very nice feature! I was just thinking this would be great. Does this also makes sense within the key name?

And what about using __ so that it is clear that it is an Elektra extension?

About how it is written down here: Why using '/' instead of /?

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

Or using _* would be another option. I am also not completely against **. It is, however, already used in zsh. So if we use **, it should have the same meaning.

@markus2330

markus2330 Oct 20, 2018

Contributor

Or using _* would be another option. I am also not completely against **. It is, however, already used in zsh. So if we use **, it should have the same meaning.

This comment has been minimized.

@kodebach

kodebach Oct 20, 2018

Contributor

Yes you are right we should use __ or possibly just ..., because our implementation does not support using this syntax anywhere but at the end of the pattern. To support this behaviour inside the pattern, we would need a non-standard implementation of fnmatch(3). (It might be possible with the GNU extension FNM_EXTMATCH)

Why using '/' instead of /?
If you wanted to know why I used &#47;, that is because my IDE (or more specifically clang-tidy) complains about the use of /* inside of block comments.

@kodebach

kodebach Oct 20, 2018

Contributor

Yes you are right we should use __ or possibly just ..., because our implementation does not support using this syntax anywhere but at the end of the pattern. To support this behaviour inside the pattern, we would need a non-standard implementation of fnmatch(3). (It might be possible with the GNU extension FNM_EXTMATCH)

Why using '/' instead of /?
If you wanted to know why I used &#47;, that is because my IDE (or more specifically clang-tidy) complains about the use of /* inside of block comments.

* <li> '_', when used as "/_/"(or "/_" at the end of @p pattern), matches a key part that is <b>not</b> a valid array item </li>
* <li> if the pattern ends with "&#47;**", matching key names may contain arbitrary suffixes </li>
* </ul>
*

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

How to match literal _ and #? (How to escape glob characters?)

@markus2330

markus2330 Oct 20, 2018

Contributor

How to match literal _ and #? (How to escape glob characters?)

This comment has been minimized.

@kodebach

kodebach Oct 20, 2018

Contributor

It would probably best to just use FNM_NOESCAPE and use character classes (which I will add to the documentation) for literal matching.

@kodebach

kodebach Oct 20, 2018

Contributor

It would probably best to just use FNM_NOESCAPE and use character classes (which I will add to the documentation) for literal matching.

/**
* @brief checks whether a given Key matches a given globbing pattern
*
* The globbing patterns for this function are a superset of those from fnmatch(3)

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

I think you want to point users to glob(7). If fnmatch is used, is not important.

@markus2330

markus2330 Oct 20, 2018

Contributor

I think you want to point users to glob(7). If fnmatch is used, is not important.

* The globbing patterns for this function are a superset of those from fnmatch(3)
* used with the FNM_PATHNAME flag:
* <ul>
* <li> '*' matches any series of characters other than '/'</li>

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

Please either add character classes here or only describe the Elektra extensions.

@markus2330

markus2330 Oct 20, 2018

Contributor

Please either add character classes here or only describe the Elektra extensions.

return ELEKTRA_GLOB_NOMATCH;
}
const char * ptr = pattern;

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

put to a separate function?

@markus2330

markus2330 Oct 20, 2018

Contributor

put to a separate function?

*
* @see keyGlob(), for explanation of globbing pattern
*/
int ksGlob (KeySet * result, KeySet * input, const char * pattern)

This comment has been minimized.

@markus2330

markus2330 Oct 20, 2018

Contributor

Return the result? (Like in ksCut?)

@markus2330

markus2330 Oct 20, 2018

Contributor

Return the result? (Like in ksCut?)

This comment has been minimized.

@kodebach

kodebach Oct 20, 2018

Contributor

This function is basically the same as elektraKsFilter with the filter function (in this case keyGlob) inlined.

We should either allocate a new KeySet with ksNew and return that or leave it as is, because in the current implementation you could pass a non-empty KeySet as result and get a number of appended Keys as the return value. Appending to a non-empty KeySet might be useful, if the result of several globbing operations are needed in a single KeySet, in which case this implementation would be more efficient than allocating multiple KeySets and merging them.

@kodebach

kodebach Oct 20, 2018

Contributor

This function is basically the same as elektraKsFilter with the filter function (in this case keyGlob) inlined.

We should either allocate a new KeySet with ksNew and return that or leave it as is, because in the current implementation you could pass a non-empty KeySet as result and get a number of appended Keys as the return value. Appending to a non-empty KeySet might be useful, if the result of several globbing operations are needed in a single KeySet, in which case this implementation would be more efficient than allocating multiple KeySets and merging them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment