Message ID | pull.848.v2.git.1611759716.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Refactor chunk-format into an API | expand |
On Wed, Jan 27, 2021 at 03:01:39PM +0000, Derrick Stolee via GitGitGadget wrote: > Updates in V2 > ============= > > * The method pair_chunk() now automatically sets a pointer while > read_chunk() uses the callback. This greatly reduces the code size. > > * Pointer casts are now implicit instead of explicit. > > * Extra care is taken to not overflow when verifying chunk sizes on write. Thanks, I read the range-diff between this version and the last and appreciate you taking the time to address all of my concerns. I think that this is ready to go, so please have my: Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > This is a restart on the topic previously submitted [1] but dropped because > ak/corrected-commit-date was still in progress. This version is based on > that branch. I've read the topic through, and found it a pleasant read. There are some questionable use of integer types, some uneven application of casts, and the reading side API is somewhat underdocumented, but the overall direction looked quite sane. I am undecided if I should expect a reroll, or declare what we have "already good enough" for 'next' and expect incremental refinements, though. A reroll that catches all nits would certainly make the resulting topic's history nicer for future developers to work with, but its also a pain to re-read essentially the same patches again, so...
On 2/4/2021 9:08 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> This is a restart on the topic previously submitted [1] but dropped because >> ak/corrected-commit-date was still in progress. This version is based on >> that branch. > > I've read the topic through, and found it a pleasant read. There > are some questionable use of integer types, some uneven application > of casts, and the reading side API is somewhat underdocumented, but > the overall direction looked quite sane. > > I am undecided if I should expect a reroll, or declare what we have > "already good enough" for 'next' and expect incremental refinements, > though. A reroll that catches all nits would certainly make the > resulting topic's history nicer for future developers to work with, > but its also a pain to re-read essentially the same patches again, > so... I appreciate the willingness to take the topic as-is, but I think you've given me enough interesting comments to deserve a re-roll. I will provide one tomorrow and hope that the range-diff is a pleasant read ;). Thanks, -Stolee