| Message ID | 5bc6a7660d897ca6c52eabba8fb9ecfb6304dabb.1646056423.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Commit-graph: Generation Number v2 Fixes | expand |
On Mon, Feb 28 2022, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > [...] > + GENERATION_VERSION=2 > + if test ! -z "$3" TIL this works somewhere :) I thought it *might* be unportable behavior (but didn't check at first), but it's not! We have a few such cases already. But IMO much less puzzling would be at least: if ! test -z "$3" Or in this case, more plainly: if test -n "$3" > + then > + GENERATION_VERSION=$3 > + fi > + OPTIONS= > + if test $GENERATION_VERSION -gt 1 > + then > + OPTIONS=" read_generation_data" > + fi Or actually, since we don't use $GENERATION_VERSION further down setting it to a default etc. here seems a bit odd. Perhaps something closer to: if test $# -eq 3 && test $3 -gt 1 It's also possible to be more clever as e.g.: test "${3:-2}" -gt 1 But that hardly seems worth it... > NUM_COMMITS=9 > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh > index 778fa418de2..669ddc645fa 100755 > --- a/t/t5324-split-commit-graph.sh > +++ b/t/t5324-split-commit-graph.sh > @@ -30,11 +30,16 @@ graph_read_expect() { > then > NUM_BASE=$2 > fi > + OPTIONS= > + if test -z "$3" > + then > + OPTIONS=" read_generation_data" > + fi > cat >expect <<- EOF > header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE > num_commits: $1 > chunks: oid_fanout oid_lookup commit_metadata generation_data > - options: > + options:$OPTIONS > EOF > test-tool read-graph >output && > test_cmp expect output Not a new issue, but it would be nice to have the mostly copy/pasted code in a lib-commit-graph.sh or something, but probably too distracting for this small series...
On 2/28/2022 10:30 AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Feb 28 2022, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <derrickstolee@github.com> >> [...] >> + GENERATION_VERSION=2 >> + if test ! -z "$3" > > TIL this works somewhere :) > > I thought it *might* be unportable behavior (but didn't check at > first), but it's not! We have a few such cases already. > > But IMO much less puzzling would be at least: > > if ! test -z "$3" > > Or in this case, more plainly: > > if test -n "$3" Sure, that makes sense. >> + then >> + GENERATION_VERSION=$3 >> + fi >> + OPTIONS= >> + if test $GENERATION_VERSION -gt 1 >> + then >> + OPTIONS=" read_generation_data" >> + fi > > Or actually, since we don't use $GENERATION_VERSION further down setting > it to a default etc. here seems a bit odd. Perhaps something closer to: > > if test $# -eq 3 && test $3 -gt 1 > > It's also possible to be more clever as e.g.: > > test "${3:-2}" -gt 1 > > But that hardly seems worth it... I prefer to use a variable so the code is self-documenting. >> NUM_COMMITS=9 >> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh >> index 778fa418de2..669ddc645fa 100755 >> --- a/t/t5324-split-commit-graph.sh >> +++ b/t/t5324-split-commit-graph.sh >> @@ -30,11 +30,16 @@ graph_read_expect() { >> then >> NUM_BASE=$2 >> fi >> + OPTIONS= >> + if test -z "$3" >> + then >> + OPTIONS=" read_generation_data" >> + fi >> cat >expect <<- EOF >> header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE >> num_commits: $1 >> chunks: oid_fanout oid_lookup commit_metadata generation_data >> - options: >> + options:$OPTIONS >> EOF >> test-tool read-graph >output && >> test_cmp expect output > > Not a new issue, but it would be nice to have the mostly copy/pasted > code in a lib-commit-graph.sh or something, but probably too distracting > for this small series... These cases are different enough in the needs of the test files that they cannot be shared without significant complication. Thanks, -Stolee
diff --git a/commit-graph.c b/commit-graph.c index a19bd96c2ee..8e52bb09552 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -407,6 +407,9 @@ struct commit_graph *parse_commit_graph(struct repository *r, &graph->chunk_generation_data); pair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW, &graph->chunk_generation_data_overflow); + + if (graph->chunk_generation_data) + graph->read_generation_data = 1; } if (r->settings.commit_graph_read_changed_paths) { diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index 5ed6d2a21c1..fa9d32facfb 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -48,7 +48,7 @@ graph_read_expect () { header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0 num_commits: $1 chunks: oid_fanout oid_lookup commit_metadata generation_data bloom_indexes bloom_data - options: bloom(1,10,7) + options: bloom(1,10,7) read_generation_data EOF test-tool read-graph >actual && test_cmp expect actual diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index f9bffe38013..1afee1c2705 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -100,11 +100,21 @@ graph_read_expect() { OPTIONAL=" $2" NUM_CHUNKS=$((3 + $(echo "$2" | wc -w))) fi + GENERATION_VERSION=2 + if test ! -z "$3" + then + GENERATION_VERSION=$3 + fi + OPTIONS= + if test $GENERATION_VERSION -gt 1 + then + OPTIONS=" read_generation_data" + fi cat >expect <<- EOF header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0 num_commits: $1 chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL - options: + options:$OPTIONS EOF test-tool read-graph >output && test_cmp expect output @@ -498,7 +508,7 @@ test_expect_success 'git commit-graph verify' ' cd "$TRASH_DIRECTORY/full" && git rev-parse commits/8 | git -c commitGraph.generationVersion=1 commit-graph write --stdin-commits && git commit-graph verify >output && - graph_read_expect 9 extra_edges + graph_read_expect 9 extra_edges 1 ' NUM_COMMITS=9 diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 778fa418de2..669ddc645fa 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -30,11 +30,16 @@ graph_read_expect() { then NUM_BASE=$2 fi + OPTIONS= + if test -z "$3" + then + OPTIONS=" read_generation_data" + fi cat >expect <<- EOF header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE num_commits: $1 chunks: oid_fanout oid_lookup commit_metadata generation_data - options: + options:$OPTIONS EOF test-tool read-graph >output && test_cmp expect output @@ -624,7 +629,7 @@ test_expect_success 'write generation data chunk if topmost remaining layer has header: 43475048 1 $(test_oid oid_version) 5 1 num_commits: $(($NUM_SECOND_LAYER_COMMITS + $NUM_THIRD_LAYER_COMMITS + $NUM_FOURTH_LAYER_COMMITS + $NUM_FIFTH_LAYER_COMMITS)) chunks: oid_fanout oid_lookup commit_metadata generation_data - options: + options: read_generation_data EOF test_cmp expect output )