git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Karthik NayakNext: Junio C Hamano
Message 4 of 18 in “builtin/clone: allow remote helpers to detect repo”
  1. 0/2 builtin/clone: allow remote helpers to detect repoPatrick Steinhardt, Feb 27, 2024
  2. 1/2 refs/reftable: don't fail empty transactions in repo without HEADPatrick Steinhardt, Feb 27, 2024
  3. Karthik NayakFeb 28, 2024
  4. Patrick SteinhardtMar 4, 2024
  5. Junio C HamanoMar 4, 2024
  6. Patrick SteinhardtMar 5, 2024
  7. Junio C HamanoMar 5, 2024
  8. t0610: remove unused variable assignmentPatrick Steinhardt, Mar 6, 2024
  9. Justin ToblerMar 1, 2024
  10. Patrick SteinhardtMar 4, 2024
  11. 2/2 builtin/clone: allow remote helpers to detect repoPatrick Steinhardt, Feb 27, 2024
  12. Junio C HamanoFeb 27, 2024
  13. Patrick SteinhardtMar 4, 2024
  14. Junio C HamanoMar 4, 2024
  15. Mike HommeyMay 3, 2024
  16. Junio C HamanoFeb 27, 2024
  17. Junio C HamanoFeb 27, 2024
  18. Patrick SteinhardtMar 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.