| Message ID | b0414221ecad1920c84f4ab498e55edec57f06b6.1709640322.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eea0d11d6d7114c850e50fa3f247b2ecf9a330fc |
| Headers | show |
| Series | reftable: memory optimizations for reflog iteration | expand |
Patrick Steinhardt <ps@pks.im> writes: > When creating a new reflog iterator, we first have to reload the stack > that the iterator is being created. This is done so that any concurrent Nit: s/created./created for.
Karthik Nayak <karthik.188@gmail.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> When creating a new reflog iterator, we first have to reload the stack >> that the iterator is being created. This is done so that any concurrent > > Nit: s/created./created for. Yeah, I couldn't grok that sentence while reviewing it. Thanks.
On Wed, Mar 06, 2024 at 08:13:39AM -0800, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > When creating a new reflog iterator, we first have to reload the stack > > that the iterator is being created. This is done so that any concurrent > > Nit: s/created./created for. Indeed, thanks. I've fixed this locally and will wait for more comments before sending a v2. Patrick
On 2024.03.05 13:10, Patrick Steinhardt wrote: > When creating a new reflog iterator, we first have to reload the stack > that the iterator is being created. This is done so that any concurrent > writes to the stack are reflected. But `reflog_iterator_for_stack()` > always reloads the main stack, which is wrong. > > Fix this and reload the correct stack. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > refs/reftable-backend.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 249a618b5a..f04be942ac 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl > if (ret) > goto done; > > - ret = reftable_stack_reload(refs->main_stack); > + ret = reftable_stack_reload(stack); > if (ret < 0) > goto done; > > -- > 2.44.0 > Is it possible to write a test to demonstrate the bug that was fixed here, or is it too much of a race condition to reliably trigger?
On Mon, Mar 11, 2024 at 11:34:23AM -0700, Josh Steadmon wrote: > On 2024.03.05 13:10, Patrick Steinhardt wrote: > > When creating a new reflog iterator, we first have to reload the stack > > that the iterator is being created. This is done so that any concurrent > > writes to the stack are reflected. But `reflog_iterator_for_stack()` > > always reloads the main stack, which is wrong. > > > > Fix this and reload the correct stack. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > refs/reftable-backend.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > > index 249a618b5a..f04be942ac 100644 > > --- a/refs/reftable-backend.c > > +++ b/refs/reftable-backend.c > > @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl > > if (ret) > > goto done; > > > > - ret = reftable_stack_reload(refs->main_stack); > > + ret = reftable_stack_reload(stack); > > if (ret < 0) > > goto done; > > > > -- > > 2.44.0 > > > > Is it possible to write a test to demonstrate the bug that was fixed > here, or is it too much of a race condition to reliably trigger? I wouldn't really know how to test for this in a way that is even somewhat reliably, unfortunately. You have to have at least two concurrent commands, one reading and one writing, where the first command loads the worktree stack and then tries to create an iter as above. Now if this was part of the reftable library we could do it rather easily via t0032, which contains unit tests for the reftable library. But in the reftable backend it's much harder. Patrick
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 249a618b5a..f04be942ac 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl if (ret) goto done; - ret = reftable_stack_reload(refs->main_stack); + ret = reftable_stack_reload(stack); if (ret < 0) goto done;
When creating a new reflog iterator, we first have to reload the stack that the iterator is being created. This is done so that any concurrent writes to the stack are reflected. But `reflog_iterator_for_stack()` always reloads the main stack, which is wrong. Fix this and reload the correct stack. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/reftable-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)