Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 4, 2024, 06:44 UTC
- Message-ID
- <ZeVtuqEAelfiA2J9@tanuki>
- In-Reply-To
- <CAOLa=ZSycN0iYbBP-rXKW5=tNJLaSd0q8+Vm=CzNfsP2nR0sJg@mail.gmail.com>
On Wed, Feb 28, 2024 at 03:32:35AM -0800, Karthik Nayak wrote:
Show 40 quoted lines
> Patrick Steinhardt <ps@pks.im> writes: > > > Under normal circumstances, it shouldn't ever happen that a repository > > has no HEAD reference. In fact, git-update-ref(1) would fail any request > > to delete the HEAD reference, and a newly initialized repository always > > pre-creates it, too. > > > > But in the next commit, we are going to change git-clone(1) to partially > > initialize the refdb just up to the point where remote helpers can find > > the repository. With that change, we are going to run into a situation > > where repositories have no refs at all. > > > > Now there is a very particular edge case in this situation: when > > preparing an empty ref transacton, we end up returning whatever value > > `read_ref_without_reload()` returned to the caller. Under normal > > conditions this would be fine: "HEAD" should usually exist, and thus the > > function would return `0`. But if "HEAD" doesn't exist, the function > > returns a positive value which we end up returning to the caller. > > > > Fix this bug by resetting the return code to `0` and add a test. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > refs/reftable-backend.c | 1 + > > t/t0610-reftable-basics.sh | 13 +++++++++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > > index a14f2ad7f4..45568818f0 100644 > > --- a/refs/reftable-backend.c > > +++ b/refs/reftable-backend.c > > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, > > &head_referent, &head_type); > > if (ret < 0) > > goto done; > > + ret = 0; > > > > So this is not really a problem in this function, it's more of that > `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
Well, yes. I'd claim that it is a problem in this function because it returns positive even though the transaction was prepared successfully.
> Nit: would be nice to have a comment about why overriding this value is > ok.
True.
Show 17 quoted lines
> > for (i = 0; i < transaction->nr; i++) {
> > struct ref_update *u = transaction->updates[i];
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > index 6a131e40b8..c5f4d23433 100755
> > --- a/t/t0610-reftable-basics.sh
> > +++ b/t/t0610-reftable-basics.sh
> > @@ -328,6 +328,19 @@ test_expect_success 'ref transaction: writes are synced' '
> > EOF
> > '
> >
> > +test_expect_success 'ref transaction: empty transaction in empty repo' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + test_commit -C repo --no-tag A &&
> > + COMMIT=$(git -C repo rev-parse HEAD) &&
>
> why do we do this?Oh, true, this isn't actually needed.
Patrick