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 <mark@jamsek.dev>
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 <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68