From: Mark Jamsek Subject: Re: change got_object_commit_dup() return type To: Mikhail Cc: Game of Trees Date: Sun, 18 Sep 2022 02:49:45 +1000 On 22-09-12 09:18PM, Mikhail wrote: > On Mon, Sep 12, 2022 at 09:36:53AM +0200, Stefan Sperling wrote: > > On Mon, Sep 12, 2022 at 03:43:40PM +1000, Mark Jamsek wrote: > > > This can be void. We could arguably drop the function and do this > > > inline, but using the function is more consistent with Got style imo. > > > > > > ok? > > > > Yes, your patch improves the code as it is. > > > > However, the general idea of this _dup() function does not seem good. > > Callers might assume they can free the original pointer after _dup(), > > because, by convention, _dup() implies a new allocation. > > I don't think it is wise to deviate from this convention. > > > > Instead of having such a weird _dup() function, we could simply increment > > the reference counter directly where needed. > > Or maybe add new functions like got_commit_addref/delref. I don't think we need a function to decrement the reference counter as got_object_commit_close() already does this. The below diff adds got_object_commit_retain(), which increments refcnt. But I'm partial to Mikhail's "addref" name too; I thought it might be a little too close to a repository "reference" that it could be confusing for some but it might be better for those not familiar with the "retain" naming convention. ----------------------------------------------- commit ae92470c467584c47f2becb9132615c197dcda1c (fix/commitdup) from: Mark Jamsek date: Sat Sep 17 16:42:09 2022 UTC drop unconventional got_object_commit_dup() Add got_object_commit_retain() to increment commit object reference counter. diff eaf2c13e05bf70e2d0819292768dad7bbadaa8f3 ae92470c467584c47f2becb9132615c197dcda1c commit - eaf2c13e05bf70e2d0819292768dad7bbadaa8f3 commit + ae92470c467584c47f2becb9132615c197dcda1c blob - cb676e08ed97b46d2255d4e5b79240ed2b64d571 blob + 19f043e1c43b4b21c3b19d764f34a7575df5b7a7 --- include/got_object.h +++ include/got_object.h @@ -353,5 +353,5 @@ const struct got_error *got_object_tag_create(struct g const char *, struct got_object_id *, const char *, time_t, const char *, const char *, struct got_repository *, int verbosity); -const struct got_error *got_object_commit_dup(struct got_commit_object **, - struct got_commit_object *); +/* Increment commit object reference counter. */ +void got_object_commit_retain(struct got_commit_object *); blob - 90b0dd61d77d94bbc79178c6692a4fc8946ce780 blob + 51d0400aa260f449e446626d2fe0f2826612a326 --- lib/object.c +++ lib/object.c @@ -2354,11 +2354,8 @@ done: return err; } -const struct got_error * -got_object_commit_dup(struct got_commit_object **ret, - struct got_commit_object *commit) +void +got_object_commit_retain(struct got_commit_object *commit) { - *ret = commit; commit->refcnt++; - return NULL; } blob - cd0eb94e4774f59e2fb7433accbb170c140d3053 blob + c8f8e0512c8e9f6c714b6313ca065cd339a262e2 --- tog/tog.c +++ tog/tog.c @@ -2362,12 +2362,9 @@ queue_commits(struct tog_log_thread_args *a) "alloc_commit_queue_entry"); break; } + matched->commit = entry->commit; + got_object_commit_retain(entry->commit); - err = got_object_commit_dup(&matched->commit, - entry->commit); - if (err) - break; - matched->idx = a->limit_commits->ncommits; TAILQ_INSERT_TAIL(&a->limit_commits->head, matched, entry); @@ -3177,14 +3174,9 @@ limit_log_view(struct tog_view *view) "alloc_commit_queue_entry"); break; } + matched->commit = entry->commit; + got_object_commit_retain(entry->commit); - err = got_object_commit_dup(&matched->commit, - entry->commit); - if (err) { - free(matched); - return err; - } - matched->idx = s->limit_commits.ncommits; TAILQ_INSERT_TAIL(&s->limit_commits.head, matched, entry); -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68