{"thread":{"id":"48450","subject":"[PATCH 1/2] object.c: free replace map in raw_object_store_clear","startedAt":"2018-05-09T23:41:14Z","lastAt":"2018-05-10T18:32:45Z","messageCount":8,"participants":["Stefan Beller","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"347145","messageId":"20180509234059.52156-1-sbeller@google.com","threadId":"48450","inReplyTo":null,"subject":"[PATCH 1/2] object.c: free replace map in raw_object_store_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-09T23:40:58Z","receivedAt":"2018-05-09T23:41:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The replace map for objects was missed to free in the object store in\nthe conversion of 174774cd519 (Merge branch 'sb/object-store-replace',\n2018-05-08)\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n object.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/object.c b/object.c\nindex 9d5b10d5a20..ff28f90c5ef 100644\n--- a/object.c\n+++ b/object.c\n@@ -497,6 +497,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n {\n \tFREE_AND_NULL(o->objectdir);\n \tFREE_AND_NULL(o->alternate_db);\n+\tFREE_AND_NULL(o->replace_map);\n \n \tfree_alt_odbs(o);\n \to->alt_odb_tail = NULL;\n-- \n2.17.0.255.g8bfb7c0704\n\n"},{"id":"347146","messageId":"20180509234059.52156-2-sbeller@google.com","threadId":"48450","inReplyTo":"20180509234059.52156-1-sbeller@google.com","subject":"[PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-09T23:40:59Z","receivedAt":"2018-05-09T23:41:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was missed in 5982da9d2ce (replace-object: allow\nprepare_replace_object to handle arbitrary repositories, 2018-04-11)\n\nTechnically the code works correctly as the replace_map is the same\nsize in different repositories, however it is hard to read. So convert\nthe code to the familiar pattern of dereferencing the pointer that we\nassign in the sizeof itself.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n replace_object.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/replace_object.c b/replace_object.c\nindex 246b98cd4f1..801b5c16789 100644\n--- a/replace_object.c\n+++ b/replace_object.c\n@@ -37,7 +37,7 @@ static void prepare_replace_object(struct repository *r)\n \t\treturn;\n \n \tr->objects->replace_map =\n-\t\txmalloc(sizeof(*the_repository->objects->replace_map));\n+\t\txmalloc(sizeof(*r->objects->replace_map));\n \toidmap_init(r->objects->replace_map, 0);\n \n \tfor_each_replace_ref(r, register_replace_ref, NULL);\n-- \n2.17.0.255.g8bfb7c0704\n\n"},{"id":"347206","messageId":"xmqqy3grizo7.fsf@gitster-ct.c.googlers.com","threadId":"48450","inReplyTo":"20180509234059.52156-1-sbeller@google.com","subject":"Re: [PATCH 1/2] object.c: free replace map in raw_object_store_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T10:16:08Z","receivedAt":"2018-05-10T10:16:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The replace map for objects was missed to free in the object store in\n> the conversion of 174774cd519 (Merge branch 'sb/object-store-replace',\n> 2018-05-08)\n\nI need a bit of clarification wrt the above.  The above makes it\nsound like the merge needed a semantic conflict resolution (e.g. one\nside turned replace_map into a pointer while the other side added a\nplace where the containing structure is freed and now the merge\nresult needs to free the pointer in the new place that frees the\ncontaining structure, but the merge forgot to do so).  Is that what\nis going on?\n\nOr is this just a simple \"the topic that ends at 174774cd519^2 had\nthis leak that needs to be fixed by this patch; instead of rerolling\nthis is an incremental, because the topic has already been merged to\n'master' and it is too late now\"?\n\nLooking at this patch in the context of the side branch (instead of\nin the merged result) already makes sense to me, so I am guessing it\nis the latter (i.e. not a botched merge that missed semantic\nconflicts), in which case the proposed log message is a bit too\nalarming and points readers in a wrong direction.  Shouldn't it\npoint at, say, c1274495 (\"replace-object: eliminate replace objects\nprepared flag\", 2018-04-11) that turned the oidmap instance into a\npointer in raw_object_store?\n\nThanks.\n\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  object.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/object.c b/object.c\n> index 9d5b10d5a20..ff28f90c5ef 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -497,6 +497,7 @@ void raw_object_store_clear(struct raw_object_store *o)\n>  {\n>  \tFREE_AND_NULL(o->objectdir);\n>  \tFREE_AND_NULL(o->alternate_db);\n> +\tFREE_AND_NULL(o->replace_map);\n>  \n>  \tfree_alt_odbs(o);\n>  \to->alt_odb_tail = NULL;\n"},{"id":"347207","messageId":"xmqqtvrfizce.fsf@gitster-ct.c.googlers.com","threadId":"48450","inReplyTo":"20180509234059.52156-2-sbeller@google.com","subject":"Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T10:23:13Z","receivedAt":"2018-05-10T10:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This was missed in 5982da9d2ce (replace-object: allow\n> prepare_replace_object to handle arbitrary repositories, 2018-04-11)\n>\n> Technically the code works correctly as the replace_map is the same\n> size in different repositories, however it is hard to read. So convert\n> the code to the familiar pattern of dereferencing the pointer that we\n> assign in the sizeof itself.\n\n;-)\n\nWe say\n\n\tptr = xmalloc(sizeof(*ptr))\n\nis better because \n\n\tptr = xmalloc(sizeof(typeof(*ptr)))\n\nis easy to go stale unless we actually use typeof and instead say a\nconcrete type like \"struct oidmap\".\n\nThis one was doing\n\n\tptr = xmalloc(sizeof(*another_ptr))\n\nand it was OK because ptr and another_ptr happened to be of the same\ntype.  I wonder if we are making it safer, or making it more obscure\nto seasoned C programmers, if we introduced a pair of helper macros,\nperhaps like these:\n\n\t#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))\n\t#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))\n\nThe change looks obviously good.  Will queue.\n\nThanks.\n"},{"id":"347211","messageId":"20180510115623.GA20632@sigill.intra.peff.net","threadId":"48450","inReplyTo":"xmqqtvrfizce.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-10T11:56:23Z","receivedAt":"2018-05-10T11:56:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:\n\n> This one was doing\n> \n> \tptr = xmalloc(sizeof(*another_ptr))\n> \n> and it was OK because ptr and another_ptr happened to be of the same\n> type.  I wonder if we are making it safer, or making it more obscure\n> to seasoned C programmers, if we introduced a pair of helper macros,\n> perhaps like these:\n> \n> \t#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))\n> \t#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))\n\nI've often wondered that, too. It's the natural endgame of the\nALLOC_ARRAY() road we've been going down.\n\n-Peff\n"},{"id":"347245","messageId":"da155b13-b915-edb7-9bf9-6bef900cdde9@gmail.com","threadId":"48450","inReplyTo":"20180510115623.GA20632@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-05-10T14:53:23Z","receivedAt":"2018-05-10T14:53:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/2018 7:56 AM, Jeff King wrote:\n> On Thu, May 10, 2018 at 07:23:13PM +0900, Junio C Hamano wrote:\n>\n>> This one was doing\n>>\n>> \tptr = xmalloc(sizeof(*another_ptr))\n>>\n>> and it was OK because ptr and another_ptr happened to be of the same\n>> type.  I wonder if we are making it safer, or making it more obscure\n>> to seasoned C programmers, if we introduced a pair of helper macros,\n>> perhaps like these:\n>>\n>> \t#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))\n>> \t#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))\n> I've often wondered that, too. It's the natural endgame of the\n> ALLOC_ARRAY() road we've been going down.\n\nI'll chime in that I like this idea.\n\nBecause I'm trying to learn more about Coccinelle, I added the diff \nbelow and ran 'make coccicheck'. It resulted in a 1000-line patch on \n'next'. I'll refrain from sending that patch series, but it's nice to \nknow a \"simple\" transformation is possible.\n\nUsing `git grep` I see 230 instances of 'xmalloc' and 261 instances of \n'xcalloc'. After the Coccinelle transformation, these are down to 194 \nand 190, respectively, because the rest allocate in the same line as the \ndefinition. It's worth thinking about the macro pattern for those cases.\n\ndiff --git a/contrib/coccinelle/alloc.cocci b/contrib/coccinelle/alloc.cocci\nnew file mode 100644\nindex 0000000000..95f426c4dc\n--- /dev/null\n+++ b/contrib/coccinelle/alloc.cocci\n@@ -0,0 +1,13 @@\n+@@\n+expression p;\n+@@\n+- p = xmalloc(sizeof(*p));\n++ ALLOCATE(p);\n+\n+@@\n+expression c;\n+expression p;\n+@@\n+- p = xcalloc(c, sizeof(*p));\n++ CALLOCATE(p,c);\n+\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f9e4c5f9bc..5398f217d7 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -843,6 +843,9 @@ extern FILE *fopen_or_warn(const char *path, const \nchar *mode);\n   */\n  #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)\n\n+#define ALLOCATE(ptr) (ptr) = xmalloc(sizeof(*(ptr)))\n+#define CALLOCATE(ptr,cnt) (ptr) = xcalloc((cnt), sizeof(*(ptr)))\n+\n  #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), \n(alloc)))\n  #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), \nst_mult(sizeof(*(x)), (alloc)))\n\n"},{"id":"347261","messageId":"CAGZ79ka0F9s32vxLE2TRUC4HKY=CdASVdawhSeGZ=BUu-5VWvQ@mail.gmail.com","threadId":"48450","inReplyTo":"xmqqy3grizo7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] object.c: free replace map in raw_object_store_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-10T17:23:58Z","receivedAt":"2018-05-10T17:24:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Junio,\n\nOn Thu, May 10, 2018 at 3:16 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The replace map for objects was missed to free in the object store in\n>> the conversion of 174774cd519 (Merge branch 'sb/object-store-replace',\n>> 2018-05-08)\n>\n>\n> Or is this just a simple \"the topic that ends at 174774cd519^2 had\n> this leak that needs to be fixed by this patch; instead of rerolling\n> this is an incremental, because the topic has already been merged to\n> 'master' and it is too late now\"?\n\nThis is the case. I wondered if I want to point to the exact commit\n(which I have trouble pointing to as the whole topic has no memory\nleak fixes, Closest would be d88f9fdf8b2 (replace-object: move\nreplace_map to object store, 2018-04-11))\nor rather just point at the series. I did not think of the confusion\nthat might arise there.\n\n> Looking at this patch in the context of the side branch (instead of\n> in the merged result) already makes sense to me, so I am guessing it\n> is the latter (i.e. not a botched merge that missed semantic\n> conflicts), in which case the proposed log message is a bit too\n> alarming and points readers in a wrong direction.  Shouldn't it\n> point at, say, c1274495 (\"replace-object: eliminate replace objects\n> prepared flag\", 2018-04-11) that turned the oidmap instance into a\n> pointer in raw_object_store?\n\nAh, that is the best place to point at. Makes sense.\nI'll reroll this,\n\nThanks,\nStefan\n"},{"id":"347286","messageId":"CAGZ79kb_qLm2dBxDmH7-N9C9+oVArxehTC3RuM7ci292AuFmBQ@mail.gmail.com","threadId":"48450","inReplyTo":"da155b13-b915-edb7-9bf9-6bef900cdde9@gmail.com","subject":"Re: [PATCH 2/2] replace-object.c: remove the_repository from prepare_replace_object","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-10T18:32:37Z","receivedAt":"2018-05-10T18:32:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Using `git grep` I see 230 instances of 'xmalloc' and 261 instances of\n> 'xcalloc'. After the Coccinelle transformation, these are down to 194 and\n> 190, respectively, because the rest allocate in the same line as the\n> definition. It's worth thinking about the macro pattern for those cases.\n\nThanks for reporting the coccinelle experiment!\n\nAs we follow a strict declare before code, and we do not know if further\ndeclarations make use of this already, e.g. given\n\n    struct foo *f = xmalloc(sizeof(*f));\n    struct bar b = &f->baz;\n\nwe cannot split up the line declaring and assigning f, but the macro\nhas to recreate the assignment upon declaration, for that we'd\nneed to have something like\n\n    ALLOCATE_TYPE(type, name);\n\nwhich over complicates things IMHO.\n\nMaybe it is worth identifying the pattern where 'f' is not used in further\ndeclarations, such that we can make patches as\n\n-    struct foo *f = xmalloc(sizeof(*f));\n+   struct foo *f;\n    struct baz b = &unrelated;\n+\n+ ALLOCATE(f);\n+\n\nThanks,\nStefan\n"}]}