From: Junio C Hamano Date: Tue, 26 Aug 2014 21:25:53 GMT Subject: Re: [PATCH v2 18/23] refs.c: add a backend method structure with transaction functions Message-ID: In-Reply-To: <1407960907-18189-19-git-send-email-sahlberg@google.com> Ronnie Sahlberg writes: > +struct ref_be { > + transaction_begin_fn transaction_begin; > + transaction_update_sha1_fn transaction_update_sha1; > + transaction_create_sha1_fn transaction_create_sha1; > + transaction_delete_sha1_fn transaction_delete_sha1; > + transaction_update_reflog_fn transaction_update_reflog; > + transaction_commit_fn transaction_commit; > + transaction_free_fn transaction_free; > +}; > + > +extern struct ref_be *refs; The overall organization is nice, but please don't use such a short name for the systemwide default singleton instance, which should not be accessed by normal code other than via helpers that implicitly use that singleton (e.g. resolve_ref_unsafe() which invokes the method of the same name on the singleton, passing the parameters it received[*1*]). The name will be used for other things (e.g. a local variable for a collection of refs) by code that do not care about the underlying implementation of the helpers and will cause confusion later. Perhaps the_refs_backend or something? Also does the singleton have to be extern, not a static inside refs.c, perhaps with a setter function to switch it or something? [Reference] *1* A typical helper that uses the singleton looks like this: +const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, + int reading, int *flag) +{ + return refs->resolve_ref_unsafe(ref, sha1, reading, flag); +}