From: Michael Haggerty <mhagger@alum.mit.edu>
To: David Turner <dturner@twopensource.com>, git@vger.kernel.org
Subject: Re: [PATCH 11/16] refs: move duplicate check to common code
Date: Wed, 23 Dec 2015 07:27:02 +0100 [thread overview]
Message-ID: <567A3EB6.9000405@alum.mit.edu> (raw)
In-Reply-To: <1449102921-7707-12-git-send-email-dturner@twopensource.com>
On 12/03/2015 01:35 AM, David Turner wrote:
> The check for duplicate refnames in a transaction is needed for
> all backends, so move it to the common code.
>
> ref_transaction_commit_fn gains a new argument, the sorted
> string_list of affected refnames.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> refs.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> refs/files-backend.c | 57 ++++-------------------------------------
> refs/refs-internal.h | 1 +
> 3 files changed, 75 insertions(+), 54 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 1b79630..808053f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1093,6 +1093,37 @@ const char *find_descendant_ref(const char *dirname,
> return NULL;
> }
>
> +/*
> + * Return 1 if there are any duplicate refnames in the updates in
> + * `transaction`, and fill in err with an appropriate error message.
> + * Fill in `refnames` with the refnames from the transaction.
> + */
> +
> +static int ref_update_reject_duplicates(struct ref_transaction *transaction,
> + struct string_list *refnames,
> + struct strbuf *err)
I like that you extract this code into a function. Though it feels
awkward to have a function called "ref_update_reject_duplicates()" that
has a side effect of filling the names into a string list. I think it
would feel more natural to call the function get_affected_refnames(),
and treat the duplicate check as an extra bonus.
You could go even farther and split it into two functions,
void get_affected_refnames(struct ref_transaction *transaction,
struct string_list *refnames);
int ref_update_reject_duplicates(struct string_list *refnames,
struct strbuf *err);
> +{
> + int i, n = transaction->nr;
> + struct ref_update **updates;
> +
> + assert(err);
> +
> + updates = transaction->updates;
> + /* Fail if a refname appears more than once in the transaction: */
> + for (i = 0; i < n; i++)
> + string_list_append(refnames, updates[i]->refname);
> + string_list_sort(refnames);
> +
> + for (i = 1; i < n; i++)
> + if (!strcmp(refnames->items[i - 1].string, refnames->items[i].string)) {
> + strbuf_addf(err,
> + "Multiple updates for ref '%s' not allowed.",
> + refnames->items[i].string);
> + return 1;
> + }
> + return 0;
> +}
> +
> /* backend functions */
> int refs_init_db(struct strbuf *err, int shared)
> {
> @@ -1102,7 +1133,29 @@ int refs_init_db(struct strbuf *err, int shared)
> int ref_transaction_commit(struct ref_transaction *transaction,
> struct strbuf *err)
> {
> - return the_refs_backend->transaction_commit(transaction, err);
> + int ret = -1;
> + struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> +
> + assert(err);
> +
> + if (transaction->state != REF_TRANSACTION_OPEN)
> + die("BUG: commit called for transaction that is not open");
> +
> + if (!transaction->nr) {
> + transaction->state = REF_TRANSACTION_CLOSED;
> + return 0;
> + }
> +
> + if (ref_update_reject_duplicates(transaction, &affected_refnames, err)) {
> + ret = TRANSACTION_GENERIC_ERROR;
> + goto done;
> + }
> +
> + ret = the_refs_backend->transaction_commit(transaction,
> + &affected_refnames, err);
> +done:
> + string_list_clear(&affected_refnames, 0);
> + return ret;
> }
Here you are avoiding a small amount of code duplication by calling
ref_update_reject_duplicates() here rather than in the backend-specific
code. But you are doing so at the cost of having to compute
affected_refnames here and pass it (redundantly) to the backend's
transaction_commit function. This increases the coupling between these
functions, which in my opinion is worse than the small amount of code
duplication. But maybe it's just me.
The check of transaction->state, on the other hand, makes sense here.
> [...]
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
next prev parent reply other threads:[~2015-12-23 6:27 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-03 0:35 [PATCH 00/16] LMDB refs backend atop pre-vtable David Turner
2015-12-03 0:35 ` [PATCH 01/16] refs: add a backend method structure with transaction functions David Turner
2015-12-05 0:07 ` Junio C Hamano
2015-12-03 0:35 ` [PATCH 02/16] refs: add methods for misc ref operations David Turner
2015-12-11 23:33 ` Junio C Hamano
2015-12-11 23:49 ` David Turner
2015-12-11 23:39 ` Junio C Hamano
2015-12-11 23:49 ` David Turner
2015-12-12 0:23 ` Junio C Hamano
2015-12-12 0:48 ` David Turner
2015-12-18 4:06 ` Howard Chu
2015-12-03 0:35 ` [PATCH 03/16] refs: add methods for the ref iterators David Turner
2016-01-03 0:06 ` David Aguilar
2016-01-04 19:01 ` Junio C Hamano
2016-01-05 13:43 ` Michael Haggerty
2016-01-05 18:56 ` Junio C Hamano
2016-01-04 19:12 ` Ronnie Sahlberg
2016-01-04 20:26 ` Junio C Hamano
2016-01-05 1:17 ` Jeff King
2016-01-05 3:29 ` Junio C Hamano
2015-12-03 0:35 ` [PATCH 04/16] refs: add do_for_each_per_worktree_ref David Turner
2015-12-11 23:52 ` Junio C Hamano
2015-12-12 0:01 ` David Turner
2015-12-03 0:35 ` [PATCH 05/16] refs: add methods for reflog David Turner
2015-12-03 0:35 ` [PATCH 06/16] refs: add method for initial ref transaction commit David Turner
2015-12-03 0:35 ` [PATCH 07/16] refs: add method for delete_refs David Turner
2015-12-03 0:35 ` [PATCH 08/16] refs: add methods to init refs backend and db David Turner
2015-12-23 5:33 ` Michael Haggerty
2015-12-23 6:54 ` David Turner
2015-12-03 0:35 ` [PATCH 09/16] refs: add method to rename refs David Turner
2015-12-03 0:35 ` [PATCH 10/16] refs: make lock generic David Turner
2015-12-03 0:35 ` [PATCH 11/16] refs: move duplicate check to common code David Turner
2015-12-23 6:27 ` Michael Haggerty [this message]
2016-01-05 16:42 ` David Turner
2015-12-03 0:35 ` [PATCH 12/16] refs: always handle non-normal refs in files backend David Turner
2015-12-23 8:02 ` Michael Haggerty
2016-01-06 0:13 ` David Turner
2016-01-06 23:41 ` [PATCH/RFC v2 1/3] refs: allow log-only updates David Turner
2016-01-06 23:41 ` [PATCH/RFC v2 2/3] refs: resolve symbolic refs first David Turner
2016-01-06 23:41 ` [PATCH/RFC v2 3/3] refs: always handle non-normal refs in files backend David Turner
2016-01-08 12:52 ` David Turner
2016-01-06 23:42 ` [PATCH 12/16] " David Turner
2015-12-03 0:35 ` [PATCH 13/16] init: allow alternate backends to be set for new repos David Turner
2015-12-05 0:07 ` Junio C Hamano
2015-12-05 6:30 ` Duy Nguyen
2015-12-05 7:44 ` Jeff King
2015-12-08 0:38 ` David Turner
2015-12-23 9:52 ` Michael Haggerty
2015-12-23 20:01 ` Jeff King
2015-12-10 18:02 ` Jeff King
2015-12-10 19:36 ` David Turner
2015-12-23 11:30 ` [PATCH] clone: use child_process for recursive checkouts Michael Haggerty
2016-01-06 23:41 ` David Turner
2015-12-23 13:34 ` [PATCH 13/16] init: allow alternate backends to be set for new repos Michael Haggerty
2016-01-05 17:26 ` David Turner
2016-01-05 18:03 ` Junio C Hamano
2016-01-05 18:24 ` David Turner
2016-01-06 12:02 ` Michael Haggerty
2016-01-06 12:52 ` Duy Nguyen
2016-01-07 3:31 ` Shawn Pearce
2015-12-03 0:35 ` [PATCH 14/16] refs: allow ref backend to be set for clone David Turner
2015-12-23 13:51 ` Michael Haggerty
2015-12-23 20:23 ` Eric Sunshine
2015-12-03 0:35 ` [PATCH 15/16] refs: add LMDB refs backend David Turner
2015-12-05 0:08 ` Junio C Hamano
2015-12-05 0:25 ` David Turner
2015-12-17 1:00 ` Jonathan Nieder
2015-12-17 2:31 ` David Turner
2015-12-17 20:49 ` Jonathan Nieder
2015-12-23 14:32 ` Michael Haggerty
2016-01-08 16:05 ` David Turner
2015-12-03 0:35 ` [PATCH 16/16] refs: tests for lmdb backend David Turner
2015-12-22 23:56 ` [PATCH 00/16] LMDB refs backend atop pre-vtable David Turner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=567A3EB6.9000405@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).