{"thread":{"id":"47403","subject":"[PATCH] decorate: clean up and document API","startedAt":"2017-12-08T00:14:51Z","lastAt":"2017-12-15T10:03:59Z","messageCount":4,"participants":["Jonathan Tan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334392","messageId":"20171208001424.81712-1-jonathantanmy@google.com","threadId":"47403","inReplyTo":null,"subject":"[PATCH] decorate: clean up and document API","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-12-08T00:14:24Z","receivedAt":"2017-12-08T00:14:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Improve the names of the identifiers in decorate.h, document them, and\nadd an example of how to use these functions.\n\nThe example is compiled and run as part of the test suite.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis patch contains some example code in a test helper.\n\nThere is a discussion at [1] about where example code for hashmap should\ngo. For something relatively simple, like decorate, perhaps example code\nisn't necessary; but if we include example code anyway, I think that we\nmight as well make it compiled and tested, so this patch contains that\nexample code in a test helper.\n\n[1] https://public-inbox.org/git/466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schindelin@gmx.de/\n---\n Documentation/technical/api-decorate.txt |  6 ---\n Makefile                                 |  1 +\n builtin/fast-export.c                    |  2 +-\n decorate.c                               | 28 ++++++------\n decorate.h                               | 49 +++++++++++++++++++--\n t/helper/.gitignore                      |  1 +\n t/helper/test-example-decorate.c         | 74 ++++++++++++++++++++++++++++++++\n t/t9004-example.sh                       | 10 +++++\n 8 files changed, 146 insertions(+), 25 deletions(-)\n delete mode 100644 Documentation/technical/api-decorate.txt\n create mode 100644 t/helper/test-example-decorate.c\n create mode 100755 t/t9004-example.sh\n\ndiff --git a/Documentation/technical/api-decorate.txt b/Documentation/technical/api-decorate.txt\ndeleted file mode 100644\nindex 1d52a6ce1..000000000\n--- a/Documentation/technical/api-decorate.txt\n+++ /dev/null\n@@ -1,6 +0,0 @@\n-decorate API\n-============\n-\n-Talk about <decorate.h>\n-\n-(Linus)\ndiff --git a/Makefile b/Makefile\nindex fef9c8d27..df52cc1c3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -651,6 +651,7 @@ TEST_PROGRAMS_NEED_X += test-dump-cache-tree\n TEST_PROGRAMS_NEED_X += test-dump-fsmonitor\n TEST_PROGRAMS_NEED_X += test-dump-split-index\n TEST_PROGRAMS_NEED_X += test-dump-untracked-cache\n+TEST_PROGRAMS_NEED_X += test-example-decorate\n TEST_PROGRAMS_NEED_X += test-fake-ssh\n TEST_PROGRAMS_NEED_X += test-genrandom\n TEST_PROGRAMS_NEED_X += test-hashmap\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex f8fe04ca5..796d0cd66 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -895,7 +895,7 @@ static void export_marks(char *file)\n {\n \tunsigned int i;\n \tuint32_t mark;\n-\tstruct object_decoration *deco = idnums.hash;\n+\tstruct decoration_entry *deco = idnums.entries;\n \tFILE *f;\n \tint e = 0;\n \ndiff --git a/decorate.c b/decorate.c\nindex 270eb2519..de31331fa 100644\n--- a/decorate.c\n+++ b/decorate.c\n@@ -14,20 +14,20 @@ static unsigned int hash_obj(const struct object *obj, unsigned int n)\n static void *insert_decoration(struct decoration *n, const struct object *base, void *decoration)\n {\n \tint size = n->size;\n-\tstruct object_decoration *hash = n->hash;\n+\tstruct decoration_entry *entries = n->entries;\n \tunsigned int j = hash_obj(base, size);\n \n-\twhile (hash[j].base) {\n-\t\tif (hash[j].base == base) {\n-\t\t\tvoid *old = hash[j].decoration;\n-\t\t\thash[j].decoration = decoration;\n+\twhile (entries[j].base) {\n+\t\tif (entries[j].base == base) {\n+\t\t\tvoid *old = entries[j].decoration;\n+\t\t\tentries[j].decoration = decoration;\n \t\t\treturn old;\n \t\t}\n \t\tif (++j >= size)\n \t\t\tj = 0;\n \t}\n-\thash[j].base = base;\n-\thash[j].decoration = decoration;\n+\tentries[j].base = base;\n+\tentries[j].decoration = decoration;\n \tn->nr++;\n \treturn NULL;\n }\n@@ -36,24 +36,23 @@ static void grow_decoration(struct decoration *n)\n {\n \tint i;\n \tint old_size = n->size;\n-\tstruct object_decoration *old_hash = n->hash;\n+\tstruct decoration_entry *old_entries = n->entries;\n \n \tn->size = (old_size + 1000) * 3 / 2;\n-\tn->hash = xcalloc(n->size, sizeof(struct object_decoration));\n+\tn->entries = xcalloc(n->size, sizeof(struct decoration_entry));\n \tn->nr = 0;\n \n \tfor (i = 0; i < old_size; i++) {\n-\t\tconst struct object *base = old_hash[i].base;\n-\t\tvoid *decoration = old_hash[i].decoration;\n+\t\tconst struct object *base = old_entries[i].base;\n+\t\tvoid *decoration = old_entries[i].decoration;\n \n \t\tif (!decoration)\n \t\t\tcontinue;\n \t\tinsert_decoration(n, base, decoration);\n \t}\n-\tfree(old_hash);\n+\tfree(old_entries);\n }\n \n-/* Add a decoration pointer, return any old one */\n void *add_decoration(struct decoration *n, const struct object *obj,\n \t\tvoid *decoration)\n {\n@@ -64,7 +63,6 @@ void *add_decoration(struct decoration *n, const struct object *obj,\n \treturn insert_decoration(n, obj, decoration);\n }\n \n-/* Lookup a decoration pointer */\n void *lookup_decoration(struct decoration *n, const struct object *obj)\n {\n \tunsigned int j;\n@@ -74,7 +72,7 @@ void *lookup_decoration(struct decoration *n, const struct object *obj)\n \t\treturn NULL;\n \tj = hash_obj(obj, n->size);\n \tfor (;;) {\n-\t\tstruct object_decoration *ref = n->hash + j;\n+\t\tstruct decoration_entry *ref = n->entries + j;\n \t\tif (ref->base == obj)\n \t\t\treturn ref->decoration;\n \t\tif (!ref->base)\ndiff --git a/decorate.h b/decorate.h\nindex e7328044f..9014c1e99 100644\n--- a/decorate.h\n+++ b/decorate.h\n@@ -1,18 +1,61 @@\n #ifndef DECORATE_H\n #define DECORATE_H\n \n-struct object_decoration {\n+/*\n+ * A data structure that associates Git objects to void pointers. See\n+ * t/helper/test-example-decorate.c for a demonstration of how to use these\n+ * functions.\n+ */\n+\n+/*\n+ * An entry in the data structure.\n+ */\n+struct decoration_entry {\n \tconst struct object *base;\n \tvoid *decoration;\n };\n \n+/*\n+ * The data structure.\n+ *\n+ * This data structure must be zero-initialized.\n+ */\n struct decoration {\n+\t/*\n+\t * Not used by the decoration mechanism. Clients may use this for\n+\t * whatever they want.\n+\t */\n \tconst char *name;\n-\tunsigned int size, nr;\n-\tstruct object_decoration *hash;\n+\n+\t/*\n+\t * The capacity of \"entries\".\n+\t */\n+\tunsigned int size;\n+\n+\t/*\n+\t * The number of real Git objects (that is, entries with non-NULL\n+\t * \"base\").\n+\t */\n+\tunsigned int nr;\n+\n+\t/*\n+\t * The entries. This is an array of size \"size\", containing nr entries\n+\t * with non-NULL \"base\" and (size - nr) entries with NULL \"base\".\n+\t */\n+\tstruct decoration_entry *entries;\n };\n \n+/*\n+ * Add an association from the given object to the given pointer (which may be\n+ * NULL), returning the previously associated pointer. If there is no previous\n+ * association, this function returns NULL.\n+ */\n extern void *add_decoration(struct decoration *n, const struct object *obj, void *decoration);\n+\n+/*\n+ * Return the pointer associated to the given object. If there is no\n+ * association, this function returns NULL.\n+ */\n extern void *lookup_decoration(struct decoration *n, const struct object *obj);\n \n #endif\ndiff --git a/t/helper/.gitignore b/t/helper/.gitignore\nindex d02f9b39a..fff6aef22 100644\n--- a/t/helper/.gitignore\n+++ b/t/helper/.gitignore\n@@ -8,6 +8,7 @@\n /test-dump-fsmonitor\n /test-dump-split-index\n /test-dump-untracked-cache\n+/test-example-decorate\n /test-fake-ssh\n /test-scrap-cache-tree\n /test-genrandom\ndiff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c\nnew file mode 100644\nindex 000000000..90dc97a9d\n--- /dev/null\n+++ b/t/helper/test-example-decorate.c\n@@ -0,0 +1,74 @@\n+#include \"cache.h\"\n+#include \"object.h\"\n+#include \"decorate.h\"\n+\n+int cmd_main(int argc, const char **argv)\n+{\n+\tstruct decoration n;\n+\tstruct object_id one_oid = { {1} };\n+\tstruct object_id two_oid = { {2} };\n+\tstruct object_id three_oid = { {3} };\n+\tstruct object *one, *two, *three;\n+\n+\tint decoration_a, decoration_b;\n+\n+\tvoid *ret;\n+\n+\tint i, objects_noticed = 0;\n+\n+\t/*\n+\t * The struct must be zero-initialized.\n+\t */\n+\tmemset(&n, 0, sizeof(n));\n+\n+\t/*\n+\t * Add 2 objects, one with a non-NULL decoration and one with a NULL\n+\t * decoration.\n+\t */\n+\tone = lookup_unknown_object(one_oid.hash);\n+\ttwo = lookup_unknown_object(two_oid.hash);\n+\tret = add_decoration(&n, one, &decoration_a);\n+\tif (ret)\n+\t\tdie(\"BUG: when adding a brand-new object, NULL should be returned\");\n+\tret = add_decoration(&n, two, NULL);\n+\tif (ret)\n+\t\tdie(\"BUG: when adding a brand-new object, NULL should be returned\");\n+\n+\t/*\n+\t * When re-adding an already existing object, the old decoration is\n+\t * returned.\n+\t */\n+\tret = add_decoration(&n, one, NULL);\n+\tif (ret != &decoration_a)\n+\t\tdie(\"BUG: when readding an already existing object, existing decoration should be returned\");\n+\tret = add_decoration(&n, two, &decoration_b);\n+\tif (ret)\n+\t\tdie(\"BUG: when readding an already existing object, existing decoration should be returned\");\n+\n+\t/*\n+\t * Lookup returns the added declarations, or NULL if the object was\n+\t * never added.\n+\t */\n+\tret = lookup_decoration(&n, one);\n+\tif (ret)\n+\t\tdie(\"BUG: lookup should return added declaration\");\n+\tret = lookup_decoration(&n, two);\n+\tif (ret != &decoration_b)\n+\t\tdie(\"BUG: lookup should return added declaration\");\n+\tthree = lookup_unknown_object(three_oid.hash);\n+\tret = lookup_decoration(&n, three);\n+\tif (ret)\n+\t\tdie(\"BUG: lookup for unknown object should return NULL\");\n+\n+\t/*\n+\t * The user can also loop through all entries.\n+\t */\n+\tfor (i = 0; i < n.size; i++) {\n+\t\tif (n.entries[i].base)\n+\t\t\tobjects_noticed++;\n+\t}\n+\tif (objects_noticed != 2)\n+\t\tdie(\"BUG: should have 2 objects\");\n+\n+\treturn 0;\n+}\ndiff --git a/t/t9004-example.sh b/t/t9004-example.sh\nnew file mode 100755\nindex 000000000..b28a028f5\n--- /dev/null\n+++ b/t/t9004-example.sh\n@@ -0,0 +1,10 @@\n+#!/bin/sh\n+\n+test_description='check that example code compiles and runs'\n+. ./test-lib.sh\n+\n+test_expect_success 'decorate' '\n+\ttest-example-decorate\n+'\n+\n+test_done\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334404","messageId":"20171208095510.GA29626@sigill.intra.peff.net","threadId":"47403","inReplyTo":"20171208001424.81712-1-jonathantanmy@google.com","subject":"Re: [PATCH] decorate: clean up and document API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T09:55:11Z","receivedAt":"2017-12-08T09:59:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 04:14:24PM -0800, Jonathan Tan wrote:\n\n> Improve the names of the identifiers in decorate.h, document them, and\n> add an example of how to use these functions.\n> \n> The example is compiled and run as part of the test suite.\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> This patch contains some example code in a test helper.\n> \n> There is a discussion at [1] about where example code for hashmap should\n> go. For something relatively simple, like decorate, perhaps example code\n> isn't necessary; but if we include example code anyway, I think that we\n> might as well make it compiled and tested, so this patch contains that\n> example code in a test helper.\n\nI have mixed feelings. On the one hand, compiling and running the code\nensures that those things actually work. On the other hand, I expect you\ncan make a much clearer example if instead of having running code, you\nshow snippets of almost-code.\n\nE.g.:\n\n  struct decoration d = { NULL };\n\n  add_decoration(&d, obj, \"foo\");\n  ...\n  str = lookup_decoration(obj);\n\npretty much gives the relevant overview, with very little boilerplate.\nYes, it omits things like the return value of add_decoration(), but\nthose sorts of details are probably better left to the function\ndocstrings.\n\nOther than that philosophical point, the documentation you added looks\npretty good to me. Two possible improvements to the API we could do on\ntop:\n\n  1. Should there be a DECORATION_INIT macro (possibly taking the \"name\"\n     as an argument)? (Actually, the whole name thing seems like a\n     confusing and bad API design in the first place).\n\n  2. This is really just an oidmap to a void pointer. I wonder if we\n     ought to be wrapping that code (I think we still want some\n     interface so that the caller doesn't have to declare their own\n     structs).\n\n-Peff\n"},{"id":"334613","messageId":"20171211103249.e34385be4688734442659e71@google.com","threadId":"47403","inReplyTo":"20171208095510.GA29626@sigill.intra.peff.net","subject":"Re: [PATCH] decorate: clean up and document API","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-12-11T18:32:49Z","receivedAt":"2017-12-11T18:32:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, 8 Dec 2017 04:55:11 -0500\nJeff King <peff@peff.net> wrote:\n\n> I have mixed feelings. On the one hand, compiling and running the code\n> ensures that those things actually work. On the other hand, I expect you\n> can make a much clearer example if instead of having running code, you\n> show snippets of almost-code.\n> \n> E.g.:\n> \n>   struct decoration d = { NULL };\n> \n>   add_decoration(&d, obj, \"foo\");\n>   ...\n>   str = lookup_decoration(obj);\n> \n> pretty much gives the relevant overview, with very little boilerplate.\n> Yes, it omits things like the return value of add_decoration(), but\n> those sorts of details are probably better left to the function\n> docstrings.\n\nThe part about iterating over all entries should probably also be shown\ntoo. Besides that, I'm OK with having a simplified example in\ndocumentation too, but I'll wait and see if others have any opinions\nbefore making any changes.\n\n> Other than that philosophical point, the documentation you added looks\n> pretty good to me. Two possible improvements to the API we could do on\n> top:\n> \n>   1. Should there be a DECORATION_INIT macro (possibly taking the \"name\"\n>      as an argument)? (Actually, the whole name thing seems like a\n>      confusing and bad API design in the first place).\n\nAgreed about the \"name\" thing. I'll add a DECORATION_INIT when I make\nthe next reroll, but I think that having it with no argument is best\n(and instantiating \"name\" with NULL).\n\n>   2. This is really just an oidmap to a void pointer. I wonder if we\n>      ought to be wrapping that code (I think we still want some\n>      interface so that the caller doesn't have to declare their own\n>      structs).\n\nIt is slightly different from oidmap in that this uses \"struct object *\"\nas a key whereas oidmap uses \"struct object_id\", meaning that a user of\n\"decorate\" must already have objects allocated or be willing to allocate\nthem, whereas a user of \"oidmap\" doesn't.\n\nHaving said that, it is true that perhaps we have too many data\nstructures doing similar things.\n"},{"id":"334870","messageId":"20171215100350.GB3567@sigill.intra.peff.net","threadId":"47403","inReplyTo":"20171211103249.e34385be4688734442659e71@google.com","subject":"Re: [PATCH] decorate: clean up and document API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-15T10:03:50Z","receivedAt":"2017-12-15T10:03:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 11, 2017 at 10:32:49AM -0800, Jonathan Tan wrote:\n\n> > Other than that philosophical point, the documentation you added looks\n> > pretty good to me. Two possible improvements to the API we could do on\n> > top:\n> > \n> >   1. Should there be a DECORATION_INIT macro (possibly taking the \"name\"\n> >      as an argument)? (Actually, the whole name thing seems like a\n> >      confusing and bad API design in the first place).\n> \n> Agreed about the \"name\" thing. I'll add a DECORATION_INIT when I make\n> the next reroll, but I think that having it with no argument is best\n> (and instantiating \"name\" with NULL).\n\nThat will leave callers like the one in log-tree unable to use the\nmacro, since it uses a static initializer. I didn't dig, though. That\nmay be the only one. Most of the rest seem to just get explicitly\nzero-initialized (some via xcalloc of a larger struct, so maybe we\nshould just promise that zero-initialization is always OK).\n\n> >   2. This is really just an oidmap to a void pointer. I wonder if we\n> >      ought to be wrapping that code (I think we still want some\n> >      interface so that the caller doesn't have to declare their own\n> >      structs).\n> \n> It is slightly different from oidmap in that this uses \"struct object *\"\n> as a key whereas oidmap uses \"struct object_id\", meaning that a user of\n> \"decorate\" must already have objects allocated or be willing to allocate\n> them, whereas a user of \"oidmap\" doesn't.\n\nAh, right. I was thinking the difference was only on the \"value\" half\nbeing a pointer versus a struct.\n\nIt's nice in the current code that decorations do not incur the extra\ncost of storing the oid twice (once in the \"struct object\", and then\nagain in the map key).  OTOH, that might well be premature optimization.\n\n-Peff\n"}]}