{"thread":{"id":"59443","subject":"[PATCH 0/6] minor cleanups to fast-export --anonymize","startedAt":"2023-03-22T17:37:03Z","lastAt":"2023-03-24T19:47:05Z","messageCount":9,"participants":["Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"473941","messageId":"20230322173636.GA5643@coredump.intra.peff.net","threadId":"59443","inReplyTo":null,"subject":"[PATCH 0/6] minor cleanups to fast-export --anonymize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:36:36Z","receivedAt":"2023-03-22T17:37:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"My goal here was cleaning up some -Wunused-parameter warnings, but doing\nso required a few preparatory cleanups, one of which actually fixes a\nreally minor bug (in patch 4).\n\n [1/6]: fast-export: drop const when storing anonymized values\n [2/6]: fast-export: simplify initialization of anonymized hashmaps\n [3/6]: fast-export: factor out anonymized_entry creation\n [4/6]: fast-export: de-obfuscate --anonymize-map handling\n [5/6]: fast-export: drop data parameter from anonymous generators\n [6/6]: fast-export: drop unused parameter from anonymize_commit_message()\n\n builtin/fast-export.c | 81 ++++++++++++++++++--------------\n t/t9351-fast-export-anonymize.sh | 2 +\n 2 files changed, 47 insertions(+), 36 deletions(-)\n\n-Peff\n"},{"id":"473942","messageId":"20230322173717.GA5795@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 1/6] fast-export: drop const when storing anonymized values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:37:17Z","receivedAt":"2023-03-22T17:37:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We store anonymized values as pointers to \"const char *\", since they are\nconceptually const to callers who use them. But they are actually\nallocated strings whose memory is owned by the struct.\n\nThe ownership mismatch hasn't been a big deal since we never free() them\n(they are held until the program ends), but let's switch them to \"char *\"\nin preparation for changing that.\n\nSince most code only accesses them via anonymize_str(), it can continue\nto narrow them to \"const char *\" in its return value.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 78493c6d2bf..f422819c82a 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -110,7 +110,7 @@ static struct decoration idnums;\n static uint32_t last_idnum;\n struct anonymized_entry {\n \tstruct hashmap_entry hash;\n-\tconst char *anon;\n+\tchar *anon;\n \tconst char orig[FLEX_ARRAY];\n };\n \n-- \n2.40.0.595.g9b96b494d8c\n\n"},{"id":"473943","messageId":"20230322173804.GB5795@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 2/6] fast-export: simplify initialization of anonymized hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:38:04Z","receivedAt":"2023-03-22T17:38:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We take pains to avoid doing a lookup on a hashmap which has not been\ninitialized with hashmap_init(). That was necessary back when this code\nwas written. But hashmap_get() became safer in b7879b0ba6e (hashmap:\nallow re-use after hashmap_free(), 2020-11-02). Since then it's OK to\ncall functions on a zero-initialized table; it will just correctly\nreturn NULL, since there is no match.\n\nThis simplifies the code a little, and also lets us keep the\ninitialization line closer to when we add an entry (which is when the\nhashmap really does need to be totally initialized). That will help\nlater refactoring.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex f422819c82a..ba9ab3a97e5 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -152,25 +152,22 @@ static const char *anonymize_str(struct hashmap *map,\n \tstruct anonymized_entry_key key;\n \tstruct anonymized_entry *ret;\n \n-\tif (!map->cmpfn)\n-\t\thashmap_init(map, anonymized_entry_cmp, NULL, 0);\n-\n \thashmap_entry_init(&key.hash, memhash(orig, len));\n \tkey.orig = orig;\n \tkey.orig_len = len;\n \n \t/* First check if it's a token the user configured manually... */\n-\tif (anonymized_seeds.cmpfn)\n-\t\tret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);\n-\telse\n-\t\tret = NULL;\n+\tret = hashmap_get_entry(&anonymized_seeds, &key, hash, &key);\n \n \t/* ...otherwise check if we've already seen it in this context... */\n \tif (!ret)\n \t\tret = hashmap_get_entry(map, &key, hash, &key);\n \n \t/* ...and finally generate a new mapping if necessary */\n \tif (!ret) {\n+\t\tif (!map->cmpfn)\n+\t\t\thashmap_init(map, anonymized_entry_cmp, NULL, 0);\n+\n \t\tFLEX_ALLOC_MEM(ret, orig, orig, len);\n \t\thashmap_entry_init(&ret->hash, key.hash.hash);\n \t\tret->anon = generate(data);\n-- \n2.40.0.595.g9b96b494d8c\n\n"},{"id":"473944","messageId":"20230322174051.GA11424@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 3/6] fast-export: factor out anonymized_entry creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:40:51Z","receivedAt":"2023-03-22T17:40:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When anonymizing output, there's only one spot where we generate new\nentries to add to our hashmap: when anonymize_str() doesn't find an\nentry, we use the generate() callback to make one and add it. Let's pull\nthat into its own function in preparation for another caller.\n\nNote that we'll add one extra feature. In anonymize_str(), we know that\nwe won't find an existing entry in the hashmap (since it will only try\nto add after failing to find one). But other callers won't have the same\nbehavior, so we should catch this case and free the now-dangling entry.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 35 ++++++++++++++++++++++++++---------\n 1 file changed, 26 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex ba9ab3a97e5..5a0b63219ad 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -139,6 +139,29 @@ static int anonymized_entry_cmp(const void *cmp_data UNUSED,\n \treturn strcmp(a->orig, b->orig);\n }\n \n+static struct anonymized_entry *add_anonymized_entry(struct hashmap *map,\n+\t\t\t\t\t\t unsigned hash,\n+\t\t\t\t\t\t const char *orig, size_t len,\n+\t\t\t\t\t\t char *anon)\n+{\n+\tstruct anonymized_entry *ret, *old;\n+\n+\tif (!map->cmpfn)\n+\t\thashmap_init(map, anonymized_entry_cmp, NULL, 0);\n+\n+\tFLEX_ALLOC_MEM(ret, orig, orig, len);\n+\thashmap_entry_init(&ret->hash, hash);\n+\tret->anon = anon;\n+\told = hashmap_put_entry(map, ret, hash);\n+\n+\tif (old) {\n+\t\tfree(old->anon);\n+\t\tfree(old);\n+\t}\n+\n+\treturn ret;\n+}\n+\n /*\n * Basically keep a cache of X->Y so that we can repeatedly replace\n * the same anonymized string with another. The actual generation\n@@ -164,15 +187,9 @@ static const char *anonymize_str(struct hashmap *map,\n \t\tret = hashmap_get_entry(map, &key, hash, &key);\n \n \t/* ...and finally generate a new mapping if necessary */\n-\tif (!ret) {\n-\t\tif (!map->cmpfn)\n-\t\t\thashmap_init(map, anonymized_entry_cmp, NULL, 0);\n-\n-\t\tFLEX_ALLOC_MEM(ret, orig, orig, len);\n-\t\thashmap_entry_init(&ret->hash, key.hash.hash);\n-\t\tret->anon = generate(data);\n-\t\thashmap_put(map, &ret->hash);\n-\t}\n+\tif (!ret)\n+\t\tret = add_anonymized_entry(map, key.hash.hash,\n+\t\t\t\t\t orig, len, generate(data));\n \n \treturn ret->anon;\n }\n-- \n2.40.0.595.g9b96b494d8c\n\n"},{"id":"473945","messageId":"20230322174213.GB11424@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 4/6] fast-export: de-obfuscate --anonymize-map handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:42:13Z","receivedAt":"2023-03-22T17:43:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we handle an --anonymize-map option, we parse the orig/anon pair,\nand then feed the \"orig\" string to anonymize_str(), along with a\ngenerator function that duplicates the \"anon\" string to be cached in the\nmap.\n\nThis works, because anonymize_str() says \"ah, there is no mapping yet\nfor orig; I'll add one from the generator\". But there are some\ndownsides:\n\n 1. It's a bit too clever, as it's not obvious what the code is trying\n to do or why it works.\n\n 2. It requires allowing generator functions to take an extra void\n pointer, which is not something any of the normal callers of\n anonymize_str() want.\n\n 3. It does the wrong thing if the same token is provided twice.\n When there are conflicting options, like:\n\n git fast-export --anonymize \\\n --anonymize-map=foo:one \\\n\t --anonymize-map=foo:two\n\n we usually let the second one override the first. But by using\n anonymize_str(), which has first-one-wins logic, we do the\n opposite.\n\nSo instead of relying on anonymize_str(), let's directly add the entry\nourselves. We can tweak the tests to show that we handle overridden\noptions correctly now.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 8 ++------\n t/t9351-fast-export-anonymize.sh | 2 ++\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 5a0b63219ad..d1e7b26dc64 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -1140,11 +1140,6 @@ static void handle_deletes(void)\n \t}\n }\n \n-static char *anonymize_seed(void *data)\n-{\n-\treturn xstrdup(data);\n-}\n-\n static int parse_opt_anonymize_map(const struct option *opt,\n \t\t\t\t const char *arg, int unset)\n {\n@@ -1166,7 +1161,8 @@ static int parse_opt_anonymize_map(const struct option *opt,\n \tif (!keylen || !*value)\n \t\treturn error(_(\"--anonymize-map token cannot be empty\"));\n \n-\tanonymize_str(map, anonymize_seed, arg, keylen, (void *)value);\n+\tadd_anonymized_entry(map, memhash(arg, keylen), arg, keylen,\n+\t\t\t xstrdup(value));\n \n \treturn 0;\n }\ndiff --git a/t/t9351-fast-export-anonymize.sh b/t/t9351-fast-export-anonymize.sh\nindex 77047e250dc..156a6474847 100755\n--- a/t/t9351-fast-export-anonymize.sh\n+++ b/t/t9351-fast-export-anonymize.sh\n@@ -25,6 +25,7 @@ test_expect_success 'setup simple repo' '\n test_expect_success 'export anonymized stream' '\n \tgit fast-export --anonymize --all \\\n \t\t--anonymize-map=retain-me \\\n+\t\t--anonymize-map=xyzzy:should-not-appear \\\n \t\t--anonymize-map=xyzzy:custom-name \\\n \t\t--anonymize-map=other \\\n \t\t>stream\n@@ -41,6 +42,7 @@ test_expect_success 'stream omits path names' '\n \n test_expect_success 'stream contains user-specified names' '\n \tgrep retain-me stream &&\n+\t! grep should-not-appear stream &&\n \tgrep custom-name stream\n '\n \n-- \n2.40.0.595.g9b96b494d8c\n\n"},{"id":"473946","messageId":"20230322174251.GC11424@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 5/6] fast-export: drop data parameter from anonymous generators","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:42:51Z","receivedAt":"2023-03-22T17:44:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The anonymization code has a specific generator callback for each type\nof data (e.g., one for paths, one for oids, and so on). These all take a\n\"data\" parameter, but none of them use it for anything. Which is not\nsurprising, as the point is to generate a new name independent of any\ninput, and each function keeps its own static counter.\n\nWe added the extra pointer in d5bf91fde44 (fast-export: add a \"data\"\ncallback parameter to anonymize_str(), 2020-06-23) to handle\n--anonymize-map parsing, but that turned out to be awkward itself, and\nwas recently dropped.\n\nSo let's get rid of this \"data\" parameter that nobody is using, both\nfrom the generators and from anonymize_str() which plumbed it through.\nThis simplifies the code, and makes -Wunused-parameter happier.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d1e7b26dc64..12adf75964c 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -168,9 +168,8 @@ static struct anonymized_entry *add_anonymized_entry(struct hashmap *map,\n * is farmed out to the generate function.\n */\n static const char *anonymize_str(struct hashmap *map,\n-\t\t\t\t char *(*generate)(void *),\n-\t\t\t\t const char *orig, size_t len,\n-\t\t\t\t void *data)\n+\t\t\t\t char *(*generate)(void),\n+\t\t\t\t const char *orig, size_t len)\n {\n \tstruct anonymized_entry_key key;\n \tstruct anonymized_entry *ret;\n@@ -189,7 +188,7 @@ static const char *anonymize_str(struct hashmap *map,\n \t/* ...and finally generate a new mapping if necessary */\n \tif (!ret)\n \t\tret = add_anonymized_entry(map, key.hash.hash,\n-\t\t\t\t\t orig, len, generate(data));\n+\t\t\t\t\t orig, len, generate());\n \n \treturn ret->anon;\n }\n@@ -202,12 +201,12 @@ static const char *anonymize_str(struct hashmap *map,\n */\n static void anonymize_path(struct strbuf *out, const char *path,\n \t\t\t struct hashmap *map,\n-\t\t\t char *(*generate)(void *))\n+\t\t\t char *(*generate)(void))\n {\n \twhile (*path) {\n \t\tconst char *end_of_component = strchrnul(path, '/');\n \t\tsize_t len = end_of_component - path;\n-\t\tconst char *c = anonymize_str(map, generate, path, len, NULL);\n+\t\tconst char *c = anonymize_str(map, generate, path, len);\n \t\tstrbuf_addstr(out, c);\n \t\tpath = end_of_component;\n \t\tif (*path)\n@@ -382,7 +381,7 @@ static void print_path_1(const char *path)\n \t\tprintf(\"%s\", path);\n }\n \n-static char *anonymize_path_component(void *data)\n+static char *anonymize_path_component(void)\n {\n \tstatic int counter;\n \tstruct strbuf out = STRBUF_INIT;\n@@ -404,7 +403,7 @@ static void print_path(const char *path)\n \t}\n }\n \n-static char *generate_fake_oid(void *data)\n+static char *generate_fake_oid(void)\n {\n \tstatic uint32_t counter = 1; /* avoid null oid */\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n@@ -420,7 +419,7 @@ static const char *anonymize_oid(const char *oid_hex)\n {\n \tstatic struct hashmap objs;\n \tsize_t len = strlen(oid_hex);\n-\treturn anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL);\n+\treturn anonymize_str(&objs, generate_fake_oid, oid_hex, len);\n }\n \n static void show_filemodify(struct diff_queue_struct *q,\n@@ -517,7 +516,7 @@ static const char *find_encoding(const char *begin, const char *end)\n \treturn bol;\n }\n \n-static char *anonymize_ref_component(void *data)\n+static char *anonymize_ref_component(void)\n {\n \tstatic int counter;\n \tstruct strbuf out = STRBUF_INIT;\n@@ -563,7 +562,7 @@ static char *anonymize_commit_message(const char *old)\n \treturn xstrfmt(\"subject %d\\n\\nbody\\n\", counter++);\n }\n \n-static char *anonymize_ident(void *data)\n+static char *anonymize_ident(void)\n {\n \tstatic int counter;\n \tstruct strbuf out = STRBUF_INIT;\n@@ -606,7 +605,7 @@ static void anonymize_ident_line(const char **beg, const char **end)\n \n \t\tlen = split.mail_end - split.name_begin;\n \t\tident = anonymize_str(&idents, anonymize_ident,\n-\t\t\t\t split.name_begin, len, NULL);\n+\t\t\t\t split.name_begin, len);\n \t\tstrbuf_addstr(out, ident);\n \t\tstrbuf_addch(out, ' ');\n \t\tstrbuf_add(out, split.date_begin, split.tz_end - split.date_begin);\n@@ -747,7 +746,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,\n \tshow_progress();\n }\n \n-static char *anonymize_tag(void *data)\n+static char *anonymize_tag(void)\n {\n \tstatic int counter;\n \tstruct strbuf out = STRBUF_INIT;\n@@ -809,7 +808,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\tif (message) {\n \t\t\tstatic struct hashmap tags;\n \t\t\tmessage = anonymize_str(&tags, anonymize_tag,\n-\t\t\t\t\t\tmessage, message_size, NULL);\n+\t\t\t\t\t\tmessage, message_size);\n \t\t\tmessage_size = strlen(message);\n \t\t}\n \t}\n-- \n2.40.0.595.g9b96b494d8c\n\n"},{"id":"473947","messageId":"20230322174322.GD11424@coredump.intra.peff.net","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"[PATCH 6/6] fast-export: drop unused parameter from anonymize_commit_message()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-22T17:43:22Z","receivedAt":"2023-03-22T17:44:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As the comment above the function indicates, we do not bother actually\nstoring commit messages in our anonymization map. But we still take the\nmessage as a parameter, and just ignore it. Let's stop doing that, which\nwill make -Wunused-parameter happier.\n\nSigned-off-by: Jeff King \n---\n builtin/fast-export.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 12adf75964c..f3cc5486861 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -556,7 +556,7 @@ static const char *anonymize_refname(const char *refname)\n * We do not even bother to cache commit messages, as they are unlikely\n * to be repeated verbatim, and it is not that interesting when they are.\n */\n-static char *anonymize_commit_message(const char *old)\n+static char *anonymize_commit_message(void)\n {\n \tstatic int counter;\n \treturn xstrfmt(\"subject %d\\n\\nbody\\n\", counter++);\n@@ -683,7 +683,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,\n \n \tmark_next_object(&commit->object);\n \tif (anonymize) {\n-\t\treencoded = anonymize_commit_message(message);\n+\t\treencoded = anonymize_commit_message();\n \t} else if (encoding) {\n \t\tswitch(reencode_mode) {\n \t\tcase REENCODE_YES:\n-- \n2.40.0.595.g9b96b494d8c\n"},{"id":"474103","messageId":"4931e301-2fb6-0d4b-a7bd-f65dee87071a@github.com","threadId":"59443","inReplyTo":"20230322173636.GA5643@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] minor cleanups to fast-export --anonymize","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-24T17:40:25Z","receivedAt":"2023-03-24T17:40:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/22/2023 1:36 PM, Jeff King wrote:\n> My goal here was cleaning up some -Wunused-parameter warnings, but doing\n> so required a few preparatory cleanups, one of which actually fixes a\n> really minor bug (in patch 4).\n\nI saw this was on its way to next(?) without any comment, but it\nreally doesn't need any. Each patch was clear and created a self-\nsufficient reason, but the culmination of dropping parameters and\nclearing up memory is also good.\n\nThanks,\n-Stolee\n"},{"id":"474119","messageId":"xmqqjzz6dj9q.fsf@gitster.g","threadId":"59443","inReplyTo":"4931e301-2fb6-0d4b-a7bd-f65dee87071a@github.com","subject":"Re: [PATCH 0/6] minor cleanups to fast-export --anonymize","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T19:46:57Z","receivedAt":"2023-03-24T19:47:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I saw this was on its way to next(?) without any comment, but it\n> really doesn't need any. Each patch was clear and created a self-\n> sufficient reason, but the culmination of dropping parameters and\n> clearing up memory is also good.\n\nI also thought these were self evidently ready for 'next'. Thanks\nfor a positive confirmation.\n\n"}]}