git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 0/3] Use default values from settings instead of config

From
Glen Choo <chooglen@google.com>
Date
Sep 17, 2021, 22:54 UTC
Message-ID
<20210917225459.68086-1-chooglen@google.com>
In-Reply-To
<20210913181221.42635-1-chooglen@google.com>

Thanks so much for the review, the comments have been really helpful! v2 doesn't change any business logic, it's mostly focused on incorporating the feedback around commit messages and tests.

Changes since v1:
* clean up typo in patch 1 commit message 
* document the commits that patches 1 and 2 address
* use test helpers in patch 1
* rewrite patch 2's tests so that it's easier to tell that each test
  does something different
* reword patch 3 commit message to explain the bug
* add tests to patch 3
Glen Choo (3):
  fsck: verify commit graph when implicitly enabled
  fsck: verify multi-pack-index when implictly enabled
  gc: perform incremental repack when implictly enabled
 builtin/fsck.c              |  5 ++-
 builtin/gc.c                |  5 +--
 t/t5318-commit-graph.sh     | 24 +++++++++-
 t/t5319-multi-pack-index.sh |  4 +-
 t/t7900-maintenance.sh      | 88 ++++++++++++++++++++++++++-----------
 5 files changed, 94 insertions(+), 32 deletions(-)
Range-diff against v1:
1:  6bddc4e158 ! 1:  97ab2bb529 fsck: verify commit graph when implicitly enabled
    @@ Metadata
      ## Commit message ##
         fsck: verify commit graph when implicitly enabled
     
    -    the_repository->settings() is the preferred way to get certain
    +    the_repository->settings is the preferred way to get certain
         settings (such as "core.commitGraph") because it gets default values
         from prepare_repo_settings(). However, cmd_fsck() reads the config
         directly via git_config_get_bool(), which bypasses these default values.
         This causes fsck to ignore the commit graph if "core.commitgraph" is not
    -    explicitly set in the config, even though commit graph is enabled by
    -    default.
    +    explicitly set in the config. This worked fine until commit-graph was
    +    enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by
    +    default, 2019-08-13).
     
         Replace git_config_get_bool("core.commitgraph") in fsck with the
         equivalent call to the_repository->settings.core_commit_graph.
    @@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
     +	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
     +		"incorrect checksum" &&
     +	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
    -+	git -c core.commitGraph=false fsck
    ++	test_config core.commitGraph false &&
    ++	git fsck
     +'
     +
     +test_expect_success 'git fsck (checks commit-graph when config unset)' '
    @@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
     +	git fsck &&
     +	corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
     +		"incorrect checksum" &&
    -+        git config --unset core.commitGraph &&
    ++	test_unconfig core.commitGraph &&
     +	cp commit-graph-pre-write-test $objdir/info/commit-graph &&
     +	test_must_fail git fsck
     +'
2:  3b86afc45b ! 2:  ace92453ca fsck: verify multi-pack-index when implictly enabled
    @@ Commit message
         Like the previous commit, the_repository->settings.core_multi_pack_index
         is preferable to reading "core.multiPackIndex" from the config because
         prepare_repo_settings() sets a default value for
    -    the_repository->settings.
    +    the_repository->settings. This worked fine until core.multiPackIndex was
    +    enabled by default in 18e449f86b (midx: enable core.multiPackIndex by
    +    default, 2020-09-25).
     
         Replace git_config_get_bool("core.multiPackIndex") in fsck with the
         equivalent call to the_repository->settings.multi_pack_index.
    @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix)
     
      ## t/t5319-multi-pack-index.sh ##
     @@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '
    - 		"incorrect object offset"
    - '
    - 
    --test_expect_success 'git-fsck incorrect offset' '
    -+test_expect_success 'git-fsck incorrect offset (config set to true)' '
    + test_expect_success 'git-fsck incorrect offset' '
      	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
      		"incorrect object offset" \
    - 		"git -c core.multipackindex=true fsck"
    - '
    - 
    -+test_expect_success 'git-fsck incorrect offset (config set to false)' '
    -+	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
    -+		"incorrect object offset" \
    +-		"git -c core.multipackindex=true fsck"
     +		"git -c core.multipackindex=true fsck" &&
    ++		test_must_fail git fsck &&
     +		git -c core.multipackindex=false fsck
    -+'
    -+
    -+test_expect_success 'git-fsck incorrect offset (config unset)' '
    -+	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
    -+		"incorrect object offset" \
    -+		"git fsck"
    -+'
    -+
    + '
    + 
      test_expect_success 'corrupt MIDX is not reused' '
    - 	corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
    - 		"incorrect object offset" &&
3:  d122ce3bf8 ! 3:  d6959d61c1 gc: perform incremental repack when implictly enabled
    @@ Metadata
      ## Commit message ##
         gc: perform incremental repack when implictly enabled
     
    -    builtin/gc.c has two ways of checking of multi-pack-index is enabled:
    +    builtin/gc.c has two ways of checking if multi-pack-index is enabled:
         - git_config_get_bool() in incremental_repack_auto_condition()
         - the_repository->settings.core_multi_pack_index in
           maintenance_task_incremental_repack()
     
    -    the_repository->settings.core_multi_pack_index should be preferred
    -    because it has default values from prepare_repo_settings().
    +    The two implementations have existed since the incremental-repack task
    +    was introduced in e841a79a13 (maintenance: add incremental-repack auto
    +    condition, 2020-09-25). These two values can diverge because
    +    prepare_repo_settings() enables the feature in the_repository->settings
    +    by default.
     
    -    Standardize on the_repository->settings.core_multi_pack_index to check
    -    if multi-pack-index is enabled.
    +    In the case where core.multiPackIndex is not set in the config, the auto
    +    condition would fail, causing the incremental-repack task to not be
    +    run. Because we always want to consider the default values, we should
    +    just always just use the_repository->settings.
    +
    +    Standardize on using the_repository->settings.core_multi_pack_index to
    +    check if multi-pack-index is enabled.
     
         Signed-off-by: Glen Choo <chooglen@google.com>
     
    @@ builtin/gc.c: static int maintenance_task_loose_objects(struct maintenance_run_o
      		return 0;
      
      	git_config_get_int("maintenance.incremental-repack.auto",
    +
    + ## t/t7900-maintenance.sh ##
    +@@ t/t7900-maintenance.sh: test_expect_success EXPENSIVE 'incremental-repack 2g limit' '
    + '
    + 
    + test_expect_success 'maintenance.incremental-repack.auto' '
    +-	git repack -adk &&
    +-	git config core.multiPackIndex true &&
    +-	git multi-pack-index write &&
    +-	GIT_TRACE2_EVENT="$(pwd)/midx-init.txt" git \
    +-		-c maintenance.incremental-repack.auto=1 \
    +-		maintenance run --auto --task=incremental-repack 2>/dev/null &&
    +-	test_subcommand ! git multi-pack-index write --no-progress <midx-init.txt &&
    +-	test_commit A &&
    +-	git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    +-	HEAD
    +-	^HEAD~1
    +-	EOF
    +-	GIT_TRACE2_EVENT=$(pwd)/trace-A git \
    +-		-c maintenance.incremental-repack.auto=2 \
    +-		maintenance run --auto --task=incremental-repack 2>/dev/null &&
    +-	test_subcommand ! git multi-pack-index write --no-progress <trace-A &&
    +-	test_commit B &&
    +-	git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    +-	HEAD
    +-	^HEAD~1
    +-	EOF
    +-	GIT_TRACE2_EVENT=$(pwd)/trace-B git \
    +-		-c maintenance.incremental-repack.auto=2 \
    +-		maintenance run --auto --task=incremental-repack 2>/dev/null &&
    +-	test_subcommand git multi-pack-index write --no-progress <trace-B
    ++	(
    ++		git init incremental-repack-true &&
    ++		cd incremental-repack-true &&
    ++		git config core.multiPackIndex true &&
    ++		test_commit A &&
    ++		git repack -adk &&
    ++		git multi-pack-index write &&
    ++		GIT_TRACE2_EVENT="$(pwd)/midx-init.txt" git \
    ++			-c maintenance.incremental-repack.auto=1 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand ! git multi-pack-index write --no-progress <midx-init.txt &&
    ++		test_commit B &&
    ++		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    ++		HEAD
    ++		^HEAD~1
    ++		EOF
    ++		GIT_TRACE2_EVENT=$(pwd)/trace-A git \
    ++			-c maintenance.incremental-repack.auto=2 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand ! git multi-pack-index write --no-progress <trace-A &&
    ++		test_commit C &&
    ++		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    ++		HEAD
    ++		^HEAD~1
    ++		EOF
    ++		GIT_TRACE2_EVENT=$(pwd)/trace-B git \
    ++			-c maintenance.incremental-repack.auto=2 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand git multi-pack-index write --no-progress <trace-B
    ++	)
    ++'
    ++
    ++test_expect_success 'maintenance.incremental-repack.auto (when config is unset)' '
    ++	(
    ++		git init incremental-repack-unset &&
    ++		cd incremental-repack-unset &&
    ++		test_unconfig core.multiPackIndex &&
    ++		test_commit A &&
    ++		git repack -adk &&
    ++		git multi-pack-index write &&
    ++		GIT_TRACE2_EVENT="$(pwd)/midx-init.txt" git \
    ++			-c maintenance.incremental-repack.auto=1 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand ! git multi-pack-index write --no-progress <midx-init.txt &&
    ++		test_commit B &&
    ++		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    ++		HEAD
    ++		^HEAD~1
    ++		EOF
    ++		GIT_TRACE2_EVENT=$(pwd)/trace-A git \
    ++			-c maintenance.incremental-repack.auto=2 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand ! git multi-pack-index write --no-progress <trace-A &&
    ++		test_commit C &&
    ++		git pack-objects --revs .git/objects/pack/pack <<-\EOF &&
    ++		HEAD
    ++		^HEAD~1
    ++		EOF
    ++		GIT_TRACE2_EVENT=$(pwd)/trace-B git \
    ++			-c maintenance.incremental-repack.auto=2 \
    ++			maintenance run --auto --task=incremental-repack 2>/dev/null &&
    ++		test_subcommand git multi-pack-index write --no-progress <trace-B
    ++	)
    + '
    + 
    + test_expect_success 'pack-refs task' '
-- 
2.33.0.464.g1972c5931b-goog
Previous: Taylor BlauNext: Glen Choo
Message 19 of 65 in “Use default values from settings instead of config”
  1. 0/3 Use default values from settings instead of configGlen Choo, Sep 13, 2021
  2. 1/3 fsck: verify commit graph when implicitly enabledGlen Choo, Sep 13, 2021
  3. Taylor BlauSep 13, 2021
  4. Eric SunshineSep 13, 2021
  5. Taylor BlauSep 13, 2021
  6. Glen ChooSep 13, 2021
  7. Eric SunshineSep 13, 2021
  8. Taylor BlauSep 14, 2021
  9. Eric SunshineSep 14, 2021
  10. Taylor BlauSep 14, 2021
  11. 2/3 fsck: verify multi-pack-index when implictly enabledGlen Choo, Sep 13, 2021
  12. Taylor BlauSep 13, 2021
  13. 3/3 gc: perform incremental repack when implictly enabledGlen Choo, Sep 13, 2021
  14. Taylor BlauSep 13, 2021
  15. Glen ChooSep 14, 2021
  16. Bagas SanjayaSep 14, 2021
  17. Glen ChooSep 16, 2021
  18. Taylor BlauSep 13, 2021
  19. 0/3 Use default values from settings instead of configGlen Choo, Sep 17, 2021
  20. 1/3 fsck: verify commit graph when implicitly enabledGlen Choo, Sep 17, 2021
  21. Eric SunshineSep 29, 2021
  22. Glen ChooSep 30, 2021
  23. Glen ChooSep 30, 2021
  24. Eric SunshineSep 30, 2021
  25. Glen ChooOct 1, 2021
  26. 2/3 fsck: verify multi-pack-index when implictly enabledGlen Choo, Sep 17, 2021
  27. Eric SunshineSep 29, 2021
  28. Glen ChooSep 29, 2021
  29. 3/3 gc: perform incremental repack when implictly enabledGlen Choo, Sep 17, 2021
  30. Eric SunshineSep 29, 2021
  31. Glen ChooSep 27, 2021
  32. Eric SunshineSep 29, 2021
  33. Glen ChooSep 29, 2021
  34. 0/3 Use default values from settings instead of configGlen Choo, Oct 5, 2021
  35. 1/3 fsck: verify commit graph when implicitly enabledGlen Choo, Oct 5, 2021
  36. 2/3 fsck: verify multi-pack-index when implictly enabledGlen Choo, Oct 5, 2021
  37. 3/3 gc: perform incremental repack when implictly enabledGlen Choo, Oct 5, 2021
  38. Ævar Arnfjörð BjarmasonOct 5, 2021
  39. Derrick StoleeOct 5, 2021
  40. Ævar Arnfjörð BjarmasonOct 5, 2021
  41. Glen ChooOct 5, 2021
  42. Junio C HamanoOct 9, 2021
  43. Glen ChooOct 11, 2021
  44. Junio C HamanoOct 11, 2021
  45. Glen ChooOct 11, 2021
  46. 0/3 Use default values from settings instead of configGlen Choo, Oct 12, 2021
  47. 1/3 fsck: verify commit graph when implicitly enabledGlen Choo, Oct 12, 2021
  48. 2/3 fsck: verify multi-pack-index when implictly enabledGlen Choo, Oct 12, 2021
  49. 3/3 gc: perform incremental repack when implictly enabledGlen Choo, Oct 12, 2021
  50. Junio C HamanoOct 12, 2021
  51. Ævar Arnfjörð BjarmasonOct 12, 2021
  52. Glen ChooOct 12, 2021
  53. Ævar Arnfjörð BjarmasonOct 14, 2021
  54. Derrick StoleeOct 13, 2021
  55. Ævar Arnfjörð BjarmasonOct 13, 2021
  56. Derrick StoleeOct 14, 2021
  57. Glen ChooOct 14, 2021
  58. Ævar Arnfjörð BjarmasonOct 14, 2021
  59. Junio C HamanoOct 14, 2021
  60. Junio C HamanoOct 15, 2021
  61. 0/3 Use default values from settings instead of configGlen Choo, Oct 15, 2021
  62. 1/3 fsck: verify commit graph when implicitly enabledGlen Choo, Oct 15, 2021
  63. 2/3 fsck: verify multi-pack-index when implictly enabledGlen Choo, Oct 15, 2021
  64. 3/3 gc: perform incremental repack when implictly enabledGlen Choo, Oct 15, 2021
  65. Junio C HamanoOct 15, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.