On Mon, Sep 14, 2020 at 04:31:03PM -0400, Derrick Stolee wrote: > On 9/14/2020 4:12 PM, Taylor Blau wrote: > > - This patch (attached below the scisors) instead of 12/12, and > > > > - This [1] patch instead of 10/12. > > > > [1]: https://lore.kernel.org/git/20200910154516.GA32117@nand.local/ > > > > Let me know if you'd rather have a full re-roll. > > It's getting a bit difficult to track all of these "use this instead" > patches. But, I'm not the one applying them, so maybe that's not actually > a problem. The above list is the only changes that I've made, so I'm happy if Junio wants to follow what's written there, but I'm equally happy to send a new reroll. > You might need a re-roll, anyway, as I have a few comments here: Let's take a look... > You also introduce commitGraph.maxNewFitlers here, which is not > mentioned in the commit message anywhere. In fact, it might be > good to include it as a separate patch so its implementation and > tests can be isolated from the command-line functionality. I could go either way on both of these, to be honest. I don't think there's anything interesting that isn't said in the documentation changes introduced by that commit that is worth convering there, so I'm not sue 'commitGraph.maxNewFilters' needs the additional call-out. > > +length zero Bloom filter. Overrides the `commitGraph.maxNewFilters` > > +configuration. > > We have found it valuable to demonstrate these overrides in tests. > Let's inspect your tests for this. > > > +test_bloom_filters_computed () { > > + commit_graph_args=$1 > > + rm -f "$TRASH_DIRECTORY/trace.event" && > > + GIT_TRACE2_EVENT="$TRASH_DIRECTORY/trace.event" git commit-graph write \ > > + $commit_graph_args && > > + grep "\"filter_not_computed\":$2" "$TRASH_DIRECTORY/trace.event" && > > + grep "\"filter_trunc_large\":$3" "$TRASH_DIRECTORY/trace.event" && > > + grep "\"filter_computed\":$4" "$TRASH_DIRECTORY/trace.event" > > +} > > If the arguments were moved to the last parameter, then we could do a few > interesting things here. > > test_bloom_filters_computed () { > NOT_COMPUTED="\"filter_not_computed\":$1" && > shift && > TRUNCATED="\"filter_trunc_large\":$1" && > shift && > COMPUTED="\"filter_computed\":$1" && > shift && > rm -f "$TRASH_DIRECTORY/trace.event" && > GIT_TRACE2_EVENT="$TRASH_DIRECTORY/trace.event" git commit-graph write $@ && > grep "$NOT_COMPUTED" "$TRASH_DIRECTORY/trace.event" && > grep "$TRUNCATED" "$TRASH_DIRECTORY/trace.event" && > grep "$COMPUTED" "$TRASH_DIRECTORY/trace.event" > } > > > (I have not tested this script. It might need some work.) > This would make your callers a bit cleaner-looking, for example: > > test_expect_success 'Bloom generation is limited by --max-new-filters' ' > ( > cd limits && > test_commit c2 filter && > test_commit c3 filter && > test_commit c4 no-filter && > test_bloom_filters_computed 3 0 2 \ > --reachable --changed-paths --split=replace --max-new-filters=2 > ) > ' > > At least, this looks nicer to me. Yeah, but I think we're still stuck with the test_config below unless you write "git $@" instead of "git commit-graph write $@". I don't think that I have strong feelings about this unless you do. > > +test_expect_success 'Bloom generation backfills previously-skipped filters' ' > > + # Check specifying commitGraph.maxNewFilters over "git config" works. > > + test_config -C limits commitGraph.maxNewFilters 1 && > > + ( > > + cd limits && > > + test_bloom_filters_computed "--reachable --changed-paths --split=replace" \ > > + 4 0 1 > > + ) > > +' > > Adding a case for `commitGraph.maxNewFilters=1` and `--max-new-filters=2` might > be interesting for the override rules. Potentially. I'm equally happy to do it in a follow-up series. I worry slightly about adding too many test-cases for somewhat trivial behavior. > > + > > +test_expect_success 'Bloom generation backfills empty commits' ' > > + git init empty && > > + test_when_finished "rm -fr empty" && > > + ( > > + cd empty && > > + for i in $(test_seq 1 6) > > + do > > + git commit --allow-empty -m "$i" > > + done && > > + > > + # Generate Bloom filters for empty commits 1-6, two at a time. > > + test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \ > > + 4 0 2 && > > + test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \ > > + 4 0 2 && > > + test_bloom_filters_computed "--reachable --changed-paths --max-new-filters=2" \ > > + 4 0 2 && > > I'm concerned that the max-new-filters limit (2) is a divisor > of the full number of commits (6). It might be good to add one > more commit here and test again with a limit of 2. That would > handle both "equal to limit" and "less than limit" cases. That case is already covered in the test two above this one ("Bloom generation is limited by --max-new-filters"). > Thanks, > -Stolee Thanks, Taylor
