| Message ID | cover.1692653671.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | commit-graph: fsck zero/non-zero generation number fixes | expand |
On Mon, Aug 21, 2023 at 05:34:32PM -0400, Taylor Blau wrote: > Here's a(nother) small reroll of a series that I sent which expanded on > a patch that Peff sent earlier in the thread to remove a section of > unreachable code that was noticed by Coverity in the > `verify_one_commit_graph()` function. > > The first few patches are the same, but the fourth (now final) patch is > modified to track a single example of a commit with zero and non-zero > generation to only emit the warning once at the end of processing. The end result looks good to me. I probably would have squashed at least 2+4 into one, and probably just squashed 3 into that as well. But I am OK with it as-is, too, and it is definitely diminishing returns to keep polishing it. Thanks for assembling it into a usable form. -Peff
Jeff King <peff@peff.net> writes: > The end result looks good to me. I probably would have squashed at least > 2+4 into one, and probably just squashed 3 into that as well. But I am > OK with it as-is, too, and it is definitely diminishing returns to keep > polishing it. I had the same impression. The endgame after applying all four looks very sensible but the changes necessary to fix things while keeping ZERO_EXISTS and NUMBER_EXISTS looked more or less like unnecessary detour. > Thanks for assembling it into a usable form. Yup. Will queue almost as-is (except for dropping the repeated "commit-graph" on the title of the last step). THanks.
On Mon, Aug 21, 2023 at 04:22:51PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > The end result looks good to me. I probably would have squashed at least > > 2+4 into one, and probably just squashed 3 into that as well. But I am > > OK with it as-is, too, and it is definitely diminishing returns to keep > > polishing it. > > I had the same impression. The endgame after applying all four > looks very sensible but the changes necessary to fix things while > keeping ZERO_EXISTS and NUMBER_EXISTS looked more or less like > unnecessary detour. I had a hard time picking between the two alternatives when assembling these patches myself. I ended up going with the approach here because I figured that the intermediate stages of the refactoring were sufficiently complicated that breaking them up made it easier for readers to digest the changes as a whole. > > Thanks for assembling it into a usable form. > > Yup. Will queue almost as-is (except for dropping the repeated > "commit-graph" on the title of the last step). Thank you! Thanks, Taylor
Here's a(nother) small reroll of a series that I sent which expanded on a patch that Peff sent earlier in the thread to remove a section of unreachable code that was noticed by Coverity in the `verify_one_commit_graph()` function. The first few patches are the same, but the fourth (now final) patch is modified to track a single example of a commit with zero and non-zero generation to only emit the warning once at the end of processing. Thanks as always for your review! Jeff King (1): commit-graph: verify swapped zero/non-zero generation cases Taylor Blau (3): commit-graph: introduce `commit_graph_generation_from_graph()` t/t5318-commit-graph.sh: test generation zero transitions during fsck commit-graph: commit-graph: avoid repeated mixed generation number warnings commit-graph.c | 38 ++++++++++++++++++++++++-------------- t/t5318-commit-graph.sh | 18 ++++++++++++------ 2 files changed, 36 insertions(+), 20 deletions(-) Range-diff against v2: 1: a1cc22297e = 1: c88f945a54 commit-graph: introduce `commit_graph_generation_from_graph()` 2: 38b8cd5e9f = 2: 8f8e0b6644 commit-graph: verify swapped zero/non-zero generation cases 3: d14f3ca840 = 3: 34a505dd4b t/t5318-commit-graph.sh: test generation zero transitions during fsck 4: e378fd6f93 < -: ---------- commit-graph: invert negated conditional, extract to function 5: 23bcb7d270 < -: ---------- commit-graph: avoid repeated mixed generation number warnings -: ---------- > 4: 52b49bb434 commit-graph: commit-graph: avoid repeated mixed generation number warnings