All of lore.kernel.org
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Bryan Turner <bturner@atlassian.com>,
	Waleed Khan <me@waleedkhan.name>,
	Han-Wen Nienhuys <hanwen@google.com>
Subject: Re: [PATCH v3 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction
Date: Mon, 17 Jan 2022 08:36:22 +0100	[thread overview]
Message-ID: <YeUcdorVFY7w9qR2@ncase> (raw)
In-Reply-To: <220113.86h7a7vlpc.gmgdl@evledraar.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5406 bytes --]

On Thu, Jan 13, 2022 at 01:43:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jan 13 2022, Patrick Steinhardt wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > When deleting loose refs, then we also have to delete the refs in the
> > packed backend. This is done by calling `refs_delete_refs()`, which
> > then uses the packed-backend's logic to delete refs. This doesn't allow
> > us to exercise any control over the reference transaction which is being
> > created in the packed backend, which is required in a subsequent commit.
> >
> > Extract a new function `packed_refs_delete_refs()`, which hosts most of
> > the logic to delete refs except for creating the transaction itself.
> > Like this, we can easily create the transaction in the files backend
> > and thus exert more control over it.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs/files-backend.c  | 12 +++++++++---
> >  refs/packed-backend.c | 28 +++++++++++++++++++++-------
> >  refs/packed-backend.h |  7 +++++++
> >  3 files changed, 37 insertions(+), 10 deletions(-)
> >
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 90b671025a..5795e54020 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1249,6 +1249,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
> >  {
> >  	struct files_ref_store *refs =
> >  		files_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
> > +	struct ref_transaction *transaction = NULL;
> 
> nit: "NULL initialization never needed?
> (re: https://lore.kernel.org/git/220113.86bl0gvtq3.gmgdl@evledraar.gmail.com/)

It is needed: if locking the packed-refs file fails we `goto error;` and
try to free the transaction without it having been initialized yet.

> >  	struct strbuf err = STRBUF_INIT;
> >  	int i, result = 0;
> >  
> > @@ -1258,10 +1259,14 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
> >  	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
> >  		goto error;
> >  
> > -	if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
> > -		packed_refs_unlock(refs->packed_ref_store);
> > +	transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
> > +	if (!transaction)
> > +		goto error;
> > +
> > +	result = packed_refs_delete_refs(refs->packed_ref_store,
> > +					 transaction, msg, refnames, flags);
> > +	if (result)
> >  		goto error;
> > -	}
> >  
> >  	packed_refs_unlock(refs->packed_ref_store);
> >  
> > @@ -1288,6 +1293,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
> >  	else
> >  		error(_("could not delete references: %s"), err.buf);
> >  
> > +	ref_transaction_free(transaction);
> >  	strbuf_release(&err);
> >  	return -1;
> >  }
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index 67152c664e..e97d67f932 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
> >  static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
> >  			     struct string_list *refnames, unsigned int flags)
> >  {
> > -	struct packed_ref_store *refs =
> > -		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
> >  	struct strbuf err = STRBUF_INIT;
> >  	struct ref_transaction *transaction;
> > -	struct string_list_item *item;
> >  	int ret;
> >  
> > -	(void)refs; /* We need the check above, but don't use the variable */
> > -
> >  	if (!refnames->nr)
> >  		return 0;
> >  
> > @@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
> >  	if (!transaction)
> >  		return -1;
> >  
> > +	ret = packed_refs_delete_refs(ref_store, transaction,
> > +				      msg, refnames, flags);
> > +
> > +	ref_transaction_free(transaction);
> > +	return ret;
> > +}
> > +
> > +int packed_refs_delete_refs(struct ref_store *ref_store,
> > +			    struct ref_transaction *transaction,
> > +			    const char *msg,
> > +			    struct string_list *refnames,
> > +			    unsigned int flags)
> > +{
> > +	struct packed_ref_store *refs =
> > +		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
> > +	struct strbuf err = STRBUF_INIT;
> > +	struct string_list_item *item;
> > +	int ret;
> > +
> > +	(void)(refs); /* We need the check above, but don't use the variable */
> > +
> >  	for_each_string_list_item(item, refnames) {
> >  		if (ref_transaction_delete(transaction, item->string, NULL,
> >  					   flags, msg, &err)) {
> 
> I see you're just moving this code around, but FWIW we can just do this
> (also in the pre-image):
> 
> 	int packed_refs_delete_refs(...)
> 	{
> 		[declare variables]
>                 
>                 /* Assert ref store sanity */
>                 packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs")
> 
>                 [...]
> 	}
> 
> Not sure it's good to change it around just for this mostly-move, just a
> note...

I think this change is trivial enough to make while at it.

Patrick

> > @@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
> >  	}
> >  
> >  	ret = ref_transaction_commit(transaction, &err);
> > -
> 
> Stray whitespace change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2022-01-17  7:36 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-07 10:55 [PATCH 0/6] refs: excessive hook execution with packed refs Patrick Steinhardt
2021-12-07 10:55 ` [PATCH 1/6] refs: open-code deletion of " Patrick Steinhardt
2021-12-07 10:56 ` [PATCH 2/6] refs: allow passing flags when beginning transactions Patrick Steinhardt
2021-12-07 10:56 ` [PATCH 3/6] refs: allow skipping the reference-transaction hook Patrick Steinhardt
2021-12-07 10:56 ` [PATCH 4/6] refs: demonstrate excessive execution of " Patrick Steinhardt
2021-12-07 10:56 ` [PATCH 5/6] refs: do not execute reference-transaction hook on packing refs Patrick Steinhardt
2021-12-07 10:56 ` [PATCH 6/6] refs: skip hooks when deleting uncovered packed refs Patrick Steinhardt
2022-01-07 11:55 ` [PATCH v2 0/6] refs: excessive hook execution with " Patrick Steinhardt
2022-01-07 11:55   ` [PATCH v2 1/6] refs: open-code deletion of " Patrick Steinhardt
2022-01-08  0:57     ` Junio C Hamano
2022-01-08  1:51     ` Junio C Hamano
2022-01-13  0:24     ` Junio C Hamano
2022-01-07 11:55   ` [PATCH v2 2/6] refs: allow passing flags when beginning transactions Patrick Steinhardt
2022-01-08  1:51     ` Junio C Hamano
2022-01-07 11:55   ` [PATCH v2 3/6] refs: allow skipping the reference-transaction hook Patrick Steinhardt
2022-01-07 11:55   ` [PATCH v2 4/6] refs: demonstrate excessive execution of " Patrick Steinhardt
2022-01-08  1:31     ` Junio C Hamano
2022-01-10 12:54       ` Patrick Steinhardt
2022-01-08  5:43     ` Eric Sunshine
2022-01-07 11:55   ` [PATCH v2 5/6] refs: do not execute reference-transaction hook on packing refs Patrick Steinhardt
2022-01-07 11:55   ` [PATCH v2 6/6] refs: skip hooks when deleting uncovered packed refs Patrick Steinhardt
2022-01-08  2:01     ` Junio C Hamano
2022-01-10 13:18       ` Patrick Steinhardt
2022-01-10 18:00         ` Junio C Hamano
2022-01-07 22:17   ` [PATCH v2 0/6] refs: excessive hook execution with " Junio C Hamano
2022-01-13 18:24     ` Han-Wen Nienhuys
2022-01-17  7:18       ` Patrick Steinhardt
2022-01-17 11:37         ` Han-Wen Nienhuys
2022-01-24  7:13           ` Patrick Steinhardt
2022-01-13  6:11   ` [PATCH v3 " Patrick Steinhardt
2022-01-13  6:11     ` [PATCH v3 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction Patrick Steinhardt
2022-01-13 12:43       ` Ævar Arnfjörð Bjarmason
2022-01-17  7:36         ` Patrick Steinhardt [this message]
2022-01-13  6:11     ` [PATCH v3 2/6] refs: allow passing flags when beginning transactions Patrick Steinhardt
2022-01-13  6:11     ` [PATCH v3 3/6] refs: allow skipping the reference-transaction hook Patrick Steinhardt
2022-01-13 13:34       ` Ævar Arnfjörð Bjarmason
2022-01-17  8:03         ` Patrick Steinhardt
2022-01-13  6:11     ` [PATCH v3 4/6] refs: demonstrate excessive execution of " Patrick Steinhardt
2022-01-13  6:11     ` [PATCH v3 5/6] refs: do not execute reference-transaction hook on packing refs Patrick Steinhardt
2022-01-13 13:00       ` Ævar Arnfjörð Bjarmason
2022-01-17  7:44         ` Patrick Steinhardt
2022-01-13  6:11     ` [PATCH v3 6/6] refs: skip hooks when deleting uncovered packed refs Patrick Steinhardt
2022-01-13 13:04       ` Ævar Arnfjörð Bjarmason
2022-01-17  7:56         ` Patrick Steinhardt
2022-01-17  8:12   ` [PATCH v4 0/6] refs: excessive hook execution with " Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 1/6] refs: extract packed_refs_delete_refs() to allow control of transaction Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 2/6] refs: allow passing flags when beginning transactions Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 3/6] refs: allow skipping the reference-transaction hook Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 4/6] refs: demonstrate excessive execution of " Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 5/6] refs: do not execute reference-transaction hook on packing refs Patrick Steinhardt
2022-01-17  8:12     ` [PATCH v4 6/6] refs: skip hooks when deleting uncovered packed refs Patrick Steinhardt

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YeUcdorVFY7w9qR2@ncase \
    --to=ps@pks.im \
    --cc=avarab@gmail.com \
    --cc=bturner@atlassian.com \
    --cc=git@vger.kernel.org \
    --cc=hanwen@google.com \
    --cc=me@waleedkhan.name \
    --cc=peff@peff.net \
    /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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.