| Message ID | b0982baacf74a4501ce5c543b294bc15d6937875.1706263918.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: fix writing multi-level indices | expand |
Patrick Steinhardt <ps@pks.im> writes: > When finishing the current section we may end up writing index records > for the section to the table. The logic to do so essentially copies what > we already have in `writer_add_record()`, making this more complicated > than it really has to be. I didn't feel like this commit message made it easier for me to understand, because I interpreted words differently than you intended. Using "may end up" makes it sound like it's unexpected behavior. Also the use of "copies" implies to me it's doing a copy operation. I would rephrase it to something like: When finishing the current section some index records might be written for the section to the table. The logic to do so is essentially duplicated from what we already have in `writer_add_record()`, making this more complicated than it really has to be. Other than that, I don't have any comments about this patch series. -- Toon
Hi It looks like this isn’t in `next` yet so I’ll leave a comment. > @@ -405,6 +405,7 @@ static int writer_finish_section(struct reftable_writer *w) > w->index = NULL; > w->index_len = 0; > w->index_cap = 0; > + This part and the next quoted one seem to be an unrelated edit by `clang-format`. They could perhaps be grouped in their own patch. > - abort(); > - } > } > - for (i = 0; i < idx_len; i++) { > + > + for (i = 0; i < idx_len; i++) > strbuf_release(&idx[i].last_key); > - } > reftable_free(idx); > }
On Wed, Jan 31, 2024 at 04:55:33PM +0100, Kristoffer Haugsbakk wrote: > Hi > > It looks like this isn’t in `next` yet so I’ll leave a comment. > > > @@ -405,6 +405,7 @@ static int writer_finish_section(struct reftable_writer *w) > > w->index = NULL; > > w->index_len = 0; > > w->index_cap = 0; > > + > > This part and the next quoted one seem to be an unrelated edit by > `clang-format`. They could perhaps be grouped in their own patch. I'll just drop this newline change here, it indeed does make the reader wonder what's going on. I'll keep the other one though -- it does not quite feel sensible to move it into its own patch. Thanks! Patrick > > - abort(); > > - } > > } > > - for (i = 0; i < idx_len; i++) { > > + > > + for (i = 0; i < idx_len; i++) > > strbuf_release(&idx[i].last_key); > > - } > > reftable_free(idx); > > } > > -- > Kristoffer Haugsbakk
On Wed, Jan 31, 2024 at 02:44:38PM +0100, Toon Claes wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > When finishing the current section we may end up writing index records > > for the section to the table. The logic to do so essentially copies what > > we already have in `writer_add_record()`, making this more complicated > > than it really has to be. > > I didn't feel like this commit message made it easier for me to > understand, because I interpreted words differently than you intended. > Using "may end up" makes it sound like it's unexpected behavior. Also > the use of "copies" implies to me it's doing a copy operation. I would > rephrase it to something like: > > When finishing the current section some index records might be written > for the section to the table. The logic to do so is essentially > duplicated from what we already have in `writer_add_record()`, making > this more complicated than it really has to be. > > Other than that, I don't have any comments about this patch series. Thanks, I'll use a slightly adapted version of this. Patrick
diff --git a/reftable/writer.c b/reftable/writer.c index 5a0b87b406..2525f236b9 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -405,6 +405,7 @@ static int writer_finish_section(struct reftable_writer *w) w->index = NULL; w->index_len = 0; w->index_cap = 0; + for (i = 0; i < idx_len; i++) { struct reftable_record rec = { .type = BLOCK_TYPE_INDEX, @@ -412,26 +413,14 @@ static int writer_finish_section(struct reftable_writer *w) .idx = idx[i], }, }; - if (block_writer_add(w->block_writer, &rec) == 0) { - continue; - } - err = writer_flush_block(w); + err = writer_add_record(w, &rec); if (err < 0) return err; - - writer_reinit_block_writer(w, BLOCK_TYPE_INDEX); - - err = block_writer_add(w->block_writer, &rec); - if (err != 0) { - /* write into fresh block should always succeed - */ - abort(); - } } - for (i = 0; i < idx_len; i++) { + + for (i = 0; i < idx_len; i++) strbuf_release(&idx[i].last_key); - } reftable_free(idx); }
When finishing the current section we may end up writing index records for the section to the table. The logic to do so essentially copies what we already have in `writer_add_record()`, making this more complicated than it really has to be. Simplify the code by using `writer_add_record()` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/writer.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)