{"thread":{"id":"60680","subject":"What's cooking in git.git (Jan 2024, #01; Tue, 2)","startedAt":"2024-01-03T01:02:40Z","lastAt":"2024-02-26T14:44:55Z","messageCount":20,"participants":["Junio C Hamano","Patrick Steinhardt","Jeff King","Taylor Blau","René Scharfe","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"486227","messageId":"xmqq5y0bcjpw.fsf@gitster.g","threadId":"60680","inReplyTo":null,"subject":"What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-03T01:02:35Z","receivedAt":"2024-01-03T01:02:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here are the topics that have been cooking in my tree.  Commits\nprefixed with '+' are in 'next' (being in 'next' is a sign that a\ntopic is stable enough to be used and are candidate to be in a\nfuture release).  Commits prefixed with '-' are only in 'seen', and\naren't considered \"accepted\" at all and may be annotated with an URL\nto a message that raises issues but they are no means exhaustive.  A\ntopic without enough support may be discarded after a long period of\nno activity (of course they can be resubmit when new interests\narise).\n\nAs the past couple of weeks have been slow, topics that have been\nmerged to 'next' (and to a lessor extent, to 'master') this season\nmay have seen less scrutinizing eyes than they deserve, which might\nlead bugs and regressions caused by them discovered later, but that\nis a regular part of life.  Let's see what happens.\n\nCopies of the source code to Git live in many repositories, and the\nfollowing is a list of the ones I push into or their mirrors.  Some\nrepositories have only a subset of branches.\n\nWith maint, master, next, seen, todo:\n\n\tgit://git.kernel.org/pub/scm/git/git.git/\n\tgit://repo.or.cz/alt-git.git/\n\thttps://kernel.googlesource.com/pub/scm/git/git/\n\thttps://github.com/git/git/\n\thttps://gitlab.com/git-vcs/git/\n\nWith all the integration branches and topics broken out:\n\n\thttps://github.com/gitster/git/\n\nEven though the preformatted documentation in HTML and man format\nare not sources, they are published in these repositories for\nconvenience (replace \"htmldocs\" with \"manpages\" for the manual\npages):\n\n\tgit://git.kernel.org/pub/scm/git/git-htmldocs.git/\n\thttps://github.com/gitster/git-htmldocs.git/\n\nRelease tarballs are available at:\n\n\thttps://www.kernel.org/pub/software/scm/git/\n\n--------------------------------------------------\n[Graduated to 'master']\n\n* jc/orphan-unborn (2023-11-24) 2 commits\n  (merged to 'next' on 2023-12-21 at 030300487a)\n + orphan/unborn: fix use of 'orphan' in end-user facing messages\n + orphan/unborn: add to the glossary and use them consistently\n\n Doc updates to clarify what an \"unborn branch\" means.\n source: <xmqq4jhb977x.fsf@gitster.g>\n\n\n* jc/retire-cas-opt-name-constant (2023-12-19) 1 commit\n  (merged to 'next' on 2023-12-21 at 39ef057c8b)\n + remote.h: retire CAS_OPT_NAME\n\n Code clean-up.\n source: <xmqq5y0uc7tq.fsf@gitster.g>\n\n\n* la/trailer-cleanups (2023-12-20) 3 commits\n  (merged to 'next' on 2023-12-21 at e26ede5f55)\n + trailer: use offsets for trailer_start/trailer_end\n + trailer: find the end of the log message\n + commit: ignore_non_trailer computes number of bytes to ignore\n\n Code clean-up.\n source: <pull.1563.v5.git.1697828495.gitgitgadget@gmail.com>\n\n\n* ps/pseudo-refs (2023-12-14) 4 commits\n  (merged to 'next' on 2023-12-21 at 3460e3d667)\n + bisect: consistently write BISECT_EXPECTED_REV via the refdb\n + refs: complete list of special refs\n + refs: propagate errno when reading special refs fails\n + wt-status: read HEAD and ORIG_HEAD via the refdb\n\n Assorted changes around pseudoref handling.\n source: <cover.1702560829.git.ps@pks.im>\n\n\n* rj/status-bisect-while-rebase (2023-10-16) 1 commit\n  (merged to 'next' on 2023-12-21 at 929a027fb7)\n + status: fix branch shown when not only bisecting\n\n \"git status\" is taught to show both the branch being bisected and\n being rebased when both are in effect at the same time.\n cf. <xmqqil76kyov.fsf@gitster.g>\n source: <2e24ca9b-9c5f-f4df-b9f8-6574a714dfb2@gmail.com>\n\n\n* rs/rebase-use-strvec-pushf (2023-12-20) 1 commit\n  (merged to 'next' on 2023-12-20 at ecb190973c)\n + rebase: use strvec_pushf() for format-patch revisions\n\n Code clean-up.\n source: <4ab7431c-6c1b-448c-b4d2-e8b9be0e4eef@web.de>\n\n\n* sh/completion-with-reftable (2023-12-19) 2 commits\n  (merged to 'next' on 2023-12-20 at 7957d4aa5b)\n + completion: support pseudoref existence checks for reftables\n + completion: refactor existence checks for pseudorefs\n\n Command line completion script (in contrib/) learned to work better\n with the reftable backend.\n source: <cover.1703022850.git.stanhu@gmail.com>\n\n--------------------------------------------------\n[New Topics]\n\n* cp/sideband-array-index-comment-fix (2023-12-28) 1 commit\n - sideband.c: remove redundant 'NEEDSWORK' tag\n\n In-code comment fix.\n source: <pull.1625.v4.git.1703750460527.gitgitgadget@gmail.com>\n\n\n* ps/worktree-refdb-initialization (2023-12-28) 6 commits\n - builtin/worktree: create refdb via ref backend\n - worktree: expose interface to look up worktree by name\n - builtin/worktree: move setup of commondir file earlier\n - refs/files: skip creation of \"refs/{heads,tags}\" for worktrees\n - setup: move creation of \"refs/\" into the files backend\n - refs: prepare `refs_init_db()` for initializing worktree refs\n\n Instead of manually creating refs/ hierarchy on disk upon a\n creation of a secondary worktree, which is only usable via the\n files backend, use the refs API to populate it.\n\n May want to wait until other topics solidify a bit more.\n cf. <xmqqedf6gpt8.fsf@gitster.g>\n source: <cover.1703754513.git.ps@pks.im>\n\n--------------------------------------------------\n[Cooking]\n\n* ml/doc-merge-updates (2023-12-20) 2 commits\n  (merged to 'next' on 2023-12-28 at 7a6329a219)\n + Documentation/git-merge.txt: use backticks for command wrapping\n + Documentation/git-merge.txt: fix reference to synopsis\n\n Doc update.\n\n Will merge to 'master'.\n source: <20231220195342.17590-1-mi.al.lohmann@gmail.com>\n\n\n* cp/apply-core-filemode (2023-12-26) 3 commits\n - apply: code simplification\n - apply: correctly reverse patch's pre- and post-image mode bits\n - apply: ignore working tree filemode when !core.filemode\n\n \"git apply\" on a filesystem without filemode support have learned\n to take a hint from what is in the index for the path, even when\n not working with the \"--index\" or \"--cached\" option, when checking\n the executable bit match what is required by the preimage in the\n patch.\n\n Needs review.\n source: <20231226233218.472054-1-gitster@pobox.com>\n\n\n* jc/archive-list-with-extra-args (2023-12-21) 1 commit\n  (merged to 'next' on 2023-12-28 at 2d5c20e67f)\n + archive: \"--list\" does not take further options\n\n \"git archive --list extra garbage\" silently ignored excess command\n line parameters, which has been corrected.\n\n Will merge to 'master'.\n source: <xmqqmsu3mnix.fsf@gitster.g>\n\n\n* jk/t1006-cat-file-objectsize-disk (2023-12-21) 1 commit\n  (merged to 'next' on 2023-12-28 at d82812e636)\n + t1006: add tests for %(objectsize:disk)\n\n Test update.\n\n Will merge to 'master'.\n source: <20231221094722.GA570888@coredump.intra.peff.net>\n\n\n* js/contributor-docs-updates (2023-12-27) 9 commits\n  (merged to 'next' on 2024-01-02 at 0e072117cd)\n + SubmittingPatches: hyphenate non-ASCII\n + SubmittingPatches: clarify GitHub artifact format\n + SubmittingPatches: clarify GitHub visual\n + SubmittingPatches: provide tag naming advice\n + SubmittingPatches: update extra tags list\n + SubmittingPatches: discourage new trailers\n + SubmittingPatches: drop ref to \"What's in git.git\"\n + CodingGuidelines: write punctuation marks\n + CodingGuidelines: move period inside parentheses\n\n Doc update.\n\n Will merge to 'master'.\n source: <pull.1623.v3.git.1703739324.gitgitgadget@gmail.com>\n\n\n* al/unit-test-ctype (2024-01-02) 1 commit\n - unit-tests: rewrite t/helper/test-ctype.c as a unit test\n\n Move test-ctype helper to the unit-test framework.\n source: <20240101104017.9452-2-ach.lumap@gmail.com>\n\n\n* bk/bisect-doc-fix (2023-12-27) 1 commit\n - doc: use singular form of repeatable path arg\n\n Synopsis fix.\n\n Expecting a reroll.\n source: <3d46bca1-96d4-43ba-a912-1f7c76942287@smtp-relay.sendinblue.com>\n\n\n* en/sparse-checkout-eoo (2023-12-26) 2 commits\n  (merged to 'next' on 2023-12-28 at 3ddf2ebab6)\n + sparse-checkout: be consistent with end of options markers\n + Merge branch 'jk/end-of-options' into jc/sparse-checkout-set-add-end-of-options\n\n \"git sparse-checkout (add|set) --[no-]cone --end-of-options\" did\n not handle \"--end-of-options\" correctly after a recent update.\n\n Will merge to 'master'.\n source: <pull.1625.v2.git.git.1703619562639.gitgitgadget@gmail.com>\n\n\n* ja/doc-placeholders-fix (2023-12-26) 2 commits\n - doc: enforce placeholders in documentation\n - doc: enforce dashes in placeholders\n\n Docfix.\n\n Needs review.\n source: <pull.1626.git.1703539287.gitgitgadget@gmail.com>\n\n\n* jc/sparse-checkout-set-default-fix (2023-12-26) 1 commit\n  (merged to 'next' on 2023-12-28 at a558eccf8e)\n + sparse-checkout: use default patterns for 'set' only !stdin\n\n \"git sparse-checkout set\" added default patterns even when the\n patterns are being fed from the standard input, which has been\n corrected.\n\n Will merge to 'master'.\n source: <20231221065925.3234048-3-gitster@pobox.com>\n\n\n* rs/fast-import-simplify-mempool-allocation (2023-12-26) 1 commit\n  (merged to 'next' on 2023-12-28 at 16e6dd2a69)\n + fast-import: use mem_pool_calloc()\n\n Code simplification.\n\n Will merge to 'master'.\n source: <50c1f410-ca37-4c1c-a28b-3e9fad49f2b4@web.de>\n\n\n* rs/mem-pool-improvements (2023-12-28) 2 commits\n  (merged to 'next' on 2023-12-28 at aa03d9441c)\n + mem-pool: simplify alignment calculation\n + mem-pool: fix big allocations\n\n MemPool allocator fixes.\n\n Will merge to 'master'.\n source: <fa89d269-1a23-4ed6-bebc-30c0b629f444@web.de>\n\n\n* ps/refstorage-extension (2024-01-02) 13 commits\n - t9500: write \"extensions.refstorage\" into config\n - builtin/clone: introduce `--ref-format=` value flag\n - builtin/init: introduce `--ref-format=` value flag\n - builtin/rev-parse: introduce `--show-ref-format` flag\n - t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar\n - setup: introduce GIT_DEFAULT_REF_FORMAT envvar\n - setup: introduce \"extensions.refStorage\" extension\n - setup: set repository's formats on init\n - setup: start tracking ref storage format\n - refs: refactor logic to look up storage backends\n - worktree: skip reading HEAD when repairing worktrees\n - t: introduce DEFAULT_REPO_FORMAT prereq\n - Merge branch 'ps/clone-into-reftable-repository' into ps/refstorage-extension\n\n Introduce a new extension \"refstorage\" so that we can mark a\n repository that uses a non-default ref backend, like reftable.\n\n Will merge to 'next'?\n source: <cover.1703833818.git.ps@pks.im>\n\n\n* ps/reftable-fixes-and-optims (2023-12-28) 9 commits\n - reftable/merged: transfer ownership of records when iterating\n - reftable/merged: really reuse buffers to compute record keys\n - reftable/record: store \"val2\" hashes as static arrays\n - reftable/record: store \"val1\" hashes as static arrays\n - reftable/record: constify some parts of the interface\n - reftable/writer: fix index corruption when writing multiple indices\n - reftable/stack: do not auto-compact twice in `reftable_stack_add()`\n - reftable/stack: do not overwrite errors when compacting\n - Merge branch 'ps/reftable-fixes' into ps/reftable-fixes-and-optims\n\n More fixes and optimizations to the reftable backend.\n\n Expecting a (hopefully minor and final) reroll.\n cf. <xmqqtto2gswa.fsf@gitster.g>\n source: <cover.1703743174.git.ps@pks.im>\n\n\n* tb/multi-pack-verbatim-reuse (2023-12-14) 26 commits\n - t/perf: add performance tests for multi-pack reuse\n - pack-bitmap: enable reuse from all bitmapped packs\n - pack-objects: allow setting `pack.allowPackReuse` to \"single\"\n - t/test-lib-functions.sh: implement `test_trace2_data` helper\n - pack-objects: add tracing for various packfile metrics\n - pack-bitmap: prepare to mark objects from multiple packs for reuse\n - pack-revindex: implement `midx_pair_to_pack_pos()`\n - pack-revindex: factor out `midx_key_to_pack_pos()` helper\n - midx: implement `midx_preferred_pack()`\n - git-compat-util.h: implement checked size_t to uint32_t conversion\n - pack-objects: include number of packs reused in output\n - pack-objects: prepare `write_reused_pack_verbatim()` for multi-pack reuse\n - pack-objects: prepare `write_reused_pack()` for multi-pack reuse\n - pack-objects: pass `bitmapped_pack`'s to pack-reuse functions\n - pack-objects: keep track of `pack_start` for each reuse pack\n - pack-objects: parameterize pack-reuse routines over a single pack\n - pack-bitmap: return multiple packs via `reuse_partial_packfile_from_bitmap()`\n - pack-bitmap: simplify `reuse_partial_packfile_from_bitmap()` signature\n - ewah: implement `bitmap_is_empty()`\n - pack-bitmap: pass `bitmapped_pack` struct to pack-reuse functions\n - midx: implement `midx_locate_pack()`\n - midx: implement `BTMP` chunk\n - midx: factor out `fill_pack_info()`\n - pack-bitmap: plug leak in find_objects()\n - pack-bitmap-write: deep-clear the `bb_commit` slab\n - pack-objects: free packing_data in more places\n\n Streaming spans of packfile data used to be done only from a\n single, primary, pack in a repository with multiple packfiles.  It\n has been extended to allow reuse from other packfiles, too.\n\n Will merge to 'next'?\n cf. <ZXurD1NTZ4TAs7WZ@nand.local>\n source: <cover.1702592603.git.me@ttaylorr.com>\n\n\n* jc/bisect-doc (2023-12-09) 1 commit\n - bisect: document \"terms\" subcommand more fully\n\n Doc update.\n\n Needs review.\n source: <xmqqzfyjmk02.fsf@gitster.g>\n\n\n* en/header-cleanup (2023-12-26) 12 commits\n  (merged to 'next' on 2023-12-28 at 1ccddc2a10)\n + treewide: remove unnecessary includes in source files\n + treewide: add direct includes currently only pulled in transitively\n + trace2/tr2_tls.h: remove unnecessary include\n + submodule-config.h: remove unnecessary include\n + pkt-line.h: remove unnecessary include\n + line-log.h: remove unnecessary include\n + http.h: remove unnecessary include\n + fsmonitor--daemon.h: remove unnecessary includes\n + blame.h: remove unnecessary includes\n + archive.h: remove unnecessary include\n + treewide: remove unnecessary includes in source files\n + treewide: remove unnecessary includes from header files\n\n Remove unused header \"#include\".\n\n Will merge to 'master'.\n source: <pull.1617.v2.git.1703351700.gitgitgadget@gmail.com>\n\n\n* jw/builtin-objectmode-attr (2023-12-28) 1 commit\n  (merged to 'next' on 2024-01-02 at 4c3784b3a1)\n + attr: add builtin objectmode values support\n\n The builtin_objectmode attribute is populated for each path\n without adding anything in .gitattributes files, which would be\n useful in magic pathspec, e.g., \":(attr:builtin_objectmode=100755)\"\n to limit to executables.\n\n Will merge to 'master'.\n cf. <xmqq5y0ssknj.fsf@gitster.g>\n source: <20231116054437.2343549-1-jojwang@google.com>\n\n\n* tb/merge-tree-write-pack (2023-10-23) 5 commits\n - builtin/merge-tree.c: implement support for `--write-pack`\n - bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n - bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n - bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n - bulk-checkin: extract abstract `bulk_checkin_source`\n\n \"git merge-tree\" learned \"--write-pack\" to record its result\n without creating loose objects.\n\n Broken when an object created during a merge is needed to continue merge\n cf. <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>\n source: <cover.1698101088.git.me@ttaylorr.com>\n\n\n* tb/pair-chunk-expect (2023-11-10) 8 commits\n - midx: read `OOFF` chunk with `pair_chunk_expect()`\n - midx: read `OIDL` chunk with `pair_chunk_expect()`\n - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`\n - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`\n - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`\n - commit-graph: read `OIDL` chunk with `pair_chunk_expect()`\n - chunk-format: introduce `pair_chunk_expect()` helper\n - Merge branch 'jk/chunk-bounds-more' into HEAD\n\n Further code clean-up.\n\n Needs review.\n source: <cover.1699569246.git.me@ttaylorr.com>\n\n\n* tb/path-filter-fix (2023-10-18) 17 commits\n - bloom: introduce `deinit_bloom_filters()`\n - commit-graph: reuse existing Bloom filters where possible\n - object.h: fix mis-aligned flag bits table\n - commit-graph: drop unnecessary `graph_read_bloom_data_context`\n - commit-graph.c: unconditionally load Bloom filters\n - bloom: prepare to discard incompatible Bloom filters\n - bloom: annotate filters with hash version\n - commit-graph: new filter ver. that fixes murmur3\n - repo-settings: introduce commitgraph.changedPathsVersion\n - t4216: test changed path filters with high bit paths\n - t/helper/test-read-graph: implement `bloom-filters` mode\n - bloom.h: make `load_bloom_filter_from_graph()` public\n - t/helper/test-read-graph.c: extract `dump_graph_info()`\n - gitformat-commit-graph: describe version 2 of BDAT\n - commit-graph: ensure Bloom filters are read with consistent settings\n - revision.c: consult Bloom filters for root commits\n - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n\n The Bloom filter used for path limited history traversal was broken\n on systems whose \"char\" is unsigned; update the implementation and\n bump the format version to 2.\n\n Expecting a reroll.\n cf. <20231023202212.GA5470@szeder.dev>\n source: <cover.1697653929.git.me@ttaylorr.com>\n\n\n* ak/color-decorate-symbols (2023-10-23) 7 commits\n - log: add color.decorate.pseudoref config variable\n - refs: exempt pseudorefs from pattern prefixing\n - refs: add pseudorefs array and iteration functions\n - log: add color.decorate.ref config variable\n - log: add color.decorate.symbol config variable\n - log: use designated inits for decoration_colors\n - config: restructure color.decorate documentation\n\n A new config for coloring.\n\n Needs review.\n source: <20231023221143.72489-1-andy.koppe@gmail.com>\n\n\n* eb/hash-transition (2023-10-02) 30 commits\n - t1016-compatObjectFormat: add tests to verify the conversion between objects\n - t1006: test oid compatibility with cat-file\n - t1006: rename sha1 to oid\n - test-lib: compute the compatibility hash so tests may use it\n - builtin/ls-tree: let the oid determine the output algorithm\n - object-file: handle compat objects in check_object_signature\n - tree-walk: init_tree_desc take an oid to get the hash algorithm\n - builtin/cat-file: let the oid determine the output algorithm\n - rev-parse: add an --output-object-format parameter\n - repository: implement extensions.compatObjectFormat\n - object-file: update object_info_extended to reencode objects\n - object-file-convert: convert commits that embed signed tags\n - object-file-convert: convert commit objects when writing\n - object-file-convert: don't leak when converting tag objects\n - object-file-convert: convert tag objects when writing\n - object-file-convert: add a function to convert trees between algorithms\n - object: factor out parse_mode out of fast-import and tree-walk into in object.h\n - cache: add a function to read an OID of a specific algorithm\n - tag: sign both hashes\n - commit: export add_header_signature to support handling signatures on tags\n - commit: convert mergetag before computing the signature of a commit\n - commit: write commits for both hashes\n - object-file: add a compat_oid_in parameter to write_object_file_flags\n - object-file: update the loose object map when writing loose objects\n - loose: compatibilty short name support\n - loose: add a mapping between SHA-1 and SHA-256 for loose objects\n - repository: add a compatibility hash algorithm\n - object-names: support input of oids in any supported hash\n - oid-array: teach oid-array to handle multiple kinds of oids\n - object-file-convert: stubs for converting from one object format to another\n\n Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.\n\n Needs review.\n source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>\n\n\n* jx/remote-archive-over-smart-http (2023-12-14) 4 commits\n - archive: support remote archive from stateless transport\n - transport-helper: call do_take_over() in connect_helper\n - transport-helper: call do_take_over() in process_connect\n - transport-helper: no connection restriction in connect_helper\n\n \"git archive --remote=<remote>\" learned to talk over the smart\n http (aka stateless) transport.\n\n Needs review.\n source: <cover.1702562879.git.zhiyou.jx@alibaba-inc.com>\n\n\n* jx/sideband-chomp-newline-fix (2023-12-18) 3 commits\n - pkt-line: do not chomp newlines for sideband messages\n - pkt-line: memorize sideband fragment in reader\n - test-pkt-line: add option parser for unpack-sideband\n\n Sideband demultiplexer fixes.\n\n Will merge to 'next'?\n source: <cover.1702823801.git.zhiyou.jx@alibaba-inc.com>\n\n\n* jc/rerere-cleanup (2023-08-25) 4 commits\n - rerere: modernize use of empty strbuf\n - rerere: try_merge() should use LL_MERGE_ERROR when it means an error\n - rerere: fix comment on handle_file() helper\n - rerere: simplify check_one_conflict() helper function\n\n Code clean-up.\n\n Not ready to be reviewed yet.\n source: <20230824205456.1231371-1-gitster@pobox.com>\n\n--------------------------------------------------\n[Discarded]\n\n* jc/sparse-checkout-eoo (2023-12-21) 5 commits\n . sparse-checkout: tighten add_patterns_from_input()\n . sparse-checkout: use default patterns for 'set' only !stdin\n . SQUASH??? end-of-options test\n . sparse-checkout: take care of \"--end-of-options\" in set/add/check-rules\n + Merge branch 'jk/end-of-options' into jc/sparse-checkout-set-add-end-of-options\n\n \"git sparse-checkout (add|set) --[no-]cone --end-of-options\" did\n not handle \"--end-of-options\" correctly after a recent update.\n\n Superseded by the en/sparse-checkout-eoo topic.\n source: <20231221065925.3234048-1-gitster@pobox.com>\n"},{"id":"486231","messageId":"ZZT2WoJDg-Z-_N5P@tanuki","threadId":"60680","inReplyTo":"xmqq5y0bcjpw.fsf@gitster.g","subject":"Re: ps/refstorage-extension (was: What's cooking in git.git (Jan 2024, #01; Tue, 2))","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-03T05:53:47Z","receivedAt":"2024-01-03T05:53:52Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 02, 2024 at 05:02:35PM -0800, Junio C Hamano wrote:\n[snip]\n> * ps/refstorage-extension (2024-01-02) 13 commits\n>  - t9500: write \"extensions.refstorage\" into config\n>  - builtin/clone: introduce `--ref-format=` value flag\n>  - builtin/init: introduce `--ref-format=` value flag\n>  - builtin/rev-parse: introduce `--show-ref-format` flag\n>  - t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar\n>  - setup: introduce GIT_DEFAULT_REF_FORMAT envvar\n>  - setup: introduce \"extensions.refStorage\" extension\n>  - setup: set repository's formats on init\n>  - setup: start tracking ref storage format\n>  - refs: refactor logic to look up storage backends\n>  - worktree: skip reading HEAD when repairing worktrees\n>  - t: introduce DEFAULT_REPO_FORMAT prereq\n>  - Merge branch 'ps/clone-into-reftable-repository' into ps/refstorage-extension\n> \n>  Introduce a new extension \"refstorage\" so that we can mark a\n>  repository that uses a non-default ref backend, like reftable.\n> \n>  Will merge to 'next'?\n>  source: <cover.1703833818.git.ps@pks.im>\n\nIt's ready from my point of view, but I'm happy to wait a few days for\npeople to come back from holidays.\n\nThanks!\n\nPatrick\n"},{"id":"486249","messageId":"20240103090152.GB1866508@coredump.intra.peff.net","threadId":"60680","inReplyTo":"xmqq5y0bcjpw.fsf@gitster.g","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-03T09:01:52Z","receivedAt":"2024-01-03T09:01:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 02, 2024 at 05:02:35PM -0800, Junio C Hamano wrote:\n\n> * jk/t1006-cat-file-objectsize-disk (2023-12-21) 1 commit\n>   (merged to 'next' on 2023-12-28 at d82812e636)\n>  + t1006: add tests for %(objectsize:disk)\n> \n>  Test update.\n> \n>  Will merge to 'master'.\n>  source: <20231221094722.GA570888@coredump.intra.peff.net>\n\nIt looks like this is the original version. I posted a v2 that took\nRené's suggestion to swap out the awk for shell, but it got overlooked.\nI'm happy enough either way, but if we want to salvage that effort,\nhere's a patch which could go on top:\n\n-- >8 --\nFrom: René Scharfe <l.s.r@web.de>\nSubject: [PATCH] t1006: prefer shell loop to awk for packed object sizes\n\nTo compute the expected on-disk size of packed objects, we sort the\noutput of show-index by pack offset and then compute the difference\nbetween adjacent entries using awk. This works but has a few readability\nproblems:\n\n  1. Reading the index in pack order means don't find out the size of an\n     oid's entry until we see the _next_ entry. So we have to save it to\n     print later.\n\n     We can instead iterate in reverse order, so we compute each oid's\n     size as we see it.\n\n  2. Since the awk invocation is inside a text_expect block, we can't\n     easily use single-quotes to hold the script. So we use\n     double-quotes, but then have to escape the dollar signs in the awk\n     script.\n\n     We can swap this out for a shell loop instead (which is made much\n     easier by the first change).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI gave René authorship since this was his cleverness, but obviously I\nwrote the commit message. Giving an explicit signoff would be nice,\nthough.\n\n t/t1006-cat-file.sh | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 0c2eafae65..5ea3326128 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -1117,14 +1117,16 @@ test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n \t\twhile read idx\n \t\tdo\n \t\t\tgit show-index <\"$idx\" >idx.raw &&\n-\t\t\tsort -n <idx.raw >idx.sorted &&\n+\t\t\tsort -nr <idx.raw >idx.sorted &&\n \t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n \t\t\tend=$((packsz - rawsz)) &&\n-\t\t\tawk -v end=\"$end\" \"\n-\t\t\t  NR > 1 { print oid, \\$1 - start }\n-\t\t\t  { start = \\$1; oid = \\$2 }\n-\t\t\t  END { print oid, end - start }\n-\t\t\t\" idx.sorted ||\n+\t\t\twhile read start oid rest\n+\t\t\tdo\n+\t\t\t\tsize=$((end - start)) &&\n+\t\t\t\tend=$start &&\n+\t\t\t\techo \"$oid $size\" ||\n+\t\t\t\treturn 1\n+\t\t\tdone <idx.sorted ||\n \t\t\treturn 1\n \t\tdone\n \t} >expect.raw &&\n-- \n2.43.0.514.g7147b80757\n\n"},{"id":"486260","messageId":"xmqq8r56bcew.fsf@gitster.g","threadId":"60680","inReplyTo":"20240103090152.GB1866508@coredump.intra.peff.net","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-03T16:37:59Z","receivedAt":"2024-01-03T16:38:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It looks like this is the original version. I posted a v2 that took\n> René's suggestion to swap out the awk for shell, but it got overlooked.\n> I'm happy enough either way, but if we want to salvage that effort,\n> here's a patch which could go on top:\n\nThanks.  I was happy enough with the old one and placed the updated\none on backburner.\n\nA commit message that explains why this incremental update (i.e.,\nrewrite from awk to a shell loop) is a good idea below does make it\nworthwhile ;-)\n\n> -- >8 --\n> From: René Scharfe <l.s.r@web.de>\n> Subject: [PATCH] t1006: prefer shell loop to awk for packed object sizes\n>\n> To compute the expected on-disk size of packed objects, we sort the\n> output of show-index by pack offset and then compute the difference\n> between adjacent entries using awk. This works but has a few readability\n> problems:\n>\n>   1. Reading the index in pack order means don't find out the size of an\n>      oid's entry until we see the _next_ entry. So we have to save it to\n>      print later.\n>\n>      We can instead iterate in reverse order, so we compute each oid's\n>      size as we see it.\n\nIf you go forward, you need \"the end of the previous round\" (which\nis \"the beginning of the current round\") to be subtracted from \"the\nend of the current round\".  If you go forward, you have to have \"the\nbeginning of the previous round\" (which is \"the end of the current\nround\") from which you subtract \"the beginning of the current round\".\n\nSo from that point of view, the only difference is that you would\nnot be ready to emit in the first round, and you would need to emit\nfor the last entry after the loop.  Because we happen to have the\nend of the last entry outside the loop, we can omit the awkwardness.\n\nOK.  But iterating over a list backwards is a bit awkward ;-).\n\n>   2. Since the awk invocation is inside a text_expect block, we can't\n>      easily use single-quotes to hold the script. So we use\n>      double-quotes, but then have to escape the dollar signs in the awk\n>      script.\n\nYup.  The joy of shell quoting rules ;-)\n\n> I gave René authorship since this was his cleverness, but obviously I\n> wrote the commit message. Giving an explicit signoff would be nice,\n> though.\n\nIndeed.\n\n>  t/t1006-cat-file.sh | 14 ++++++++------\n>  1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 0c2eafae65..5ea3326128 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -1117,14 +1117,16 @@ test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n>  \t\twhile read idx\n>  \t\tdo\n>  \t\t\tgit show-index <\"$idx\" >idx.raw &&\n> -\t\t\tsort -n <idx.raw >idx.sorted &&\n> +\t\t\tsort -nr <idx.raw >idx.sorted &&\n>  \t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n>  \t\t\tend=$((packsz - rawsz)) &&\n> -\t\t\tawk -v end=\"$end\" \"\n> -\t\t\t  NR > 1 { print oid, \\$1 - start }\n> -\t\t\t  { start = \\$1; oid = \\$2 }\n> -\t\t\t  END { print oid, end - start }\n> -\t\t\t\" idx.sorted ||\n> +\t\t\twhile read start oid rest\n> +\t\t\tdo\n> +\t\t\t\tsize=$((end - start)) &&\n> +\t\t\t\tend=$start &&\n> +\t\t\t\techo \"$oid $size\" ||\n> +\t\t\t\treturn 1\n> +\t\t\tdone <idx.sorted ||\n>  \t\t\treturn 1\n>  \t\tdone\n>  \t} >expect.raw &&\n\nThis is totally unrelated tangent, but the way \"show-index\" gets\ninvoked in the above loop makes readers wonder how the caller found\nout which $idx file to read.\n\nOf course, the above loop sits downstream of a pipe\n\n    find .git/objects/pack -type f -name \\*.idx\n\nwhich means that any user of \"git show-index\" must be intimately\nfamiliar with how the object database is structured.  I wonder if we\nwant an extra layer of abstraction, similar to how the reference\ndatabase can have different backend implementation.\n\nAnyway, will queue.  Thanks.\n"},{"id":"486262","messageId":"ZZWOtnP2IHNldcy6@nand.local","threadId":"60680","inReplyTo":"xmqq5y0bcjpw.fsf@gitster.g","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-03T16:43:34Z","receivedAt":"2024-01-03T16:43:36Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 02, 2024 at 05:02:35PM -0800, Junio C Hamano wrote:\n> * tb/merge-tree-write-pack (2023-10-23) 5 commits\n>  - builtin/merge-tree.c: implement support for `--write-pack`\n>  - bulk-checkin: introduce `index_tree_bulk_checkin_incore()`\n>  - bulk-checkin: introduce `index_blob_bulk_checkin_incore()`\n>  - bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types\n>  - bulk-checkin: extract abstract `bulk_checkin_source`\n>\n>  \"git merge-tree\" learned \"--write-pack\" to record its result\n>  without creating loose objects.\n>\n>  Broken when an object created during a merge is needed to continue merge\n>  cf. <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>\n>  source: <cover.1698101088.git.me@ttaylorr.com>\n\nLet's drop this one.\n\n> * tb/pair-chunk-expect (2023-11-10) 8 commits\n>  - midx: read `OOFF` chunk with `pair_chunk_expect()`\n>  - midx: read `OIDL` chunk with `pair_chunk_expect()`\n>  - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`\n>  - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`\n>  - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`\n>  - commit-graph: read `OIDL` chunk with `pair_chunk_expect()`\n>  - chunk-format: introduce `pair_chunk_expect()` helper\n>  - Merge branch 'jk/chunk-bounds-more' into HEAD\n>\n>  Further code clean-up.\n>\n>  Needs review.\n>  source: <cover.1699569246.git.me@ttaylorr.com>\n\nThis one is on my list of things to look at, but probably not something\nthat I'll get to urgently before I've had a chance to clear my holiday\nbacklog. If you don't mind keeping it, that's fine, but I won't be upset\nif it's easier to drop from 'seen' in the meantime.\n\n> * tb/path-filter-fix (2023-10-18) 17 commits\n>  - bloom: introduce `deinit_bloom_filters()`\n>  - commit-graph: reuse existing Bloom filters where possible\n>  - object.h: fix mis-aligned flag bits table\n>  - commit-graph: drop unnecessary `graph_read_bloom_data_context`\n>  - commit-graph.c: unconditionally load Bloom filters\n>  - bloom: prepare to discard incompatible Bloom filters\n>  - bloom: annotate filters with hash version\n>  - commit-graph: new filter ver. that fixes murmur3\n>  - repo-settings: introduce commitgraph.changedPathsVersion\n>  - t4216: test changed path filters with high bit paths\n>  - t/helper/test-read-graph: implement `bloom-filters` mode\n>  - bloom.h: make `load_bloom_filter_from_graph()` public\n>  - t/helper/test-read-graph.c: extract `dump_graph_info()`\n>  - gitformat-commit-graph: describe version 2 of BDAT\n>  - commit-graph: ensure Bloom filters are read with consistent settings\n>  - revision.c: consult Bloom filters for root commits\n>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n>\n>  The Bloom filter used for path limited history traversal was broken\n>  on systems whose \"char\" is unsigned; update the implementation and\n>  bump the format version to 2.\n>\n>  Expecting a reroll.\n>  cf. <20231023202212.GA5470@szeder.dev>\n>  source: <cover.1697653929.git.me@ttaylorr.com>\n\nI was confused by this one, since I couldn't figure out which tests\nGábor was referring to here. I responded in [1], but haven't heard back\nsince the end of October.\n\nI personally think that this is ready to go, and it would be nice to get\nit out of the perpetual \"cooking\" state that it's in. So if Gábor is\naround to reply and I'm indeed missing something, that would be great.\nBut in the meantime, I think that this is ready to go.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n"},{"id":"486265","messageId":"a81ecbb1-9ebf-4026-99c3-667ad28e2c80@web.de","threadId":"60680","inReplyTo":"20240103090152.GB1866508@coredump.intra.peff.net","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-03T17:14:41Z","receivedAt":"2024-01-03T17:14:55Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.01.24 um 10:01 schrieb Jeff King:\n> On Tue, Jan 02, 2024 at 05:02:35PM -0800, Junio C Hamano wrote:\n>\n>> * jk/t1006-cat-file-objectsize-disk (2023-12-21) 1 commit\n>>   (merged to 'next' on 2023-12-28 at d82812e636)\n>>  + t1006: add tests for %(objectsize:disk)\n>>\n>>  Test update.\n>>\n>>  Will merge to 'master'.\n>>  source: <20231221094722.GA570888@coredump.intra.peff.net>\n>\n> It looks like this is the original version. I posted a v2 that took\n> René's suggestion to swap out the awk for shell, but it got overlooked.\n> I'm happy enough either way, but if we want to salvage that effort,\n> here's a patch which could go on top:\n>\n> -- >8 --\n> From: René Scharfe <l.s.r@web.de>\n> Subject: [PATCH] t1006: prefer shell loop to awk for packed object sizes\n>\n> To compute the expected on-disk size of packed objects, we sort the\n> output of show-index by pack offset and then compute the difference\n> between adjacent entries using awk. This works but has a few readability\n> problems:\n>\n>   1. Reading the index in pack order means don't find out the size of an\n>      oid's entry until we see the _next_ entry. So we have to save it to\n>      print later.\n>\n>      We can instead iterate in reverse order, so we compute each oid's\n>      size as we see it.\n>\n>   2. Since the awk invocation is inside a text_expect block, we can't\n>      easily use single-quotes to hold the script. So we use\n>      double-quotes, but then have to escape the dollar signs in the awk\n>      script.\n>\n>      We can swap this out for a shell loop instead (which is made much\n>      easier by the first change).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I gave René authorship since this was his cleverness, but obviously I\n> wrote the commit message. Giving an explicit signoff would be nice,\n> though.\n\nAlright, thank you!\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n\n>\n>  t/t1006-cat-file.sh | 14 ++++++++------\n>  1 file changed, 8 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 0c2eafae65..5ea3326128 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -1117,14 +1117,16 @@ test_expect_success 'cat-file %(objectsize:disk) with --batch-all-objects' '\n>  \t\twhile read idx\n>  \t\tdo\n>  \t\t\tgit show-index <\"$idx\" >idx.raw &&\n> -\t\t\tsort -n <idx.raw >idx.sorted &&\n> +\t\t\tsort -nr <idx.raw >idx.sorted &&\n>  \t\t\tpacksz=$(test_file_size \"${idx%.idx}.pack\") &&\n>  \t\t\tend=$((packsz - rawsz)) &&\n> -\t\t\tawk -v end=\"$end\" \"\n> -\t\t\t  NR > 1 { print oid, \\$1 - start }\n> -\t\t\t  { start = \\$1; oid = \\$2 }\n> -\t\t\t  END { print oid, end - start }\n> -\t\t\t\" idx.sorted ||\n> +\t\t\twhile read start oid rest\n> +\t\t\tdo\n> +\t\t\t\tsize=$((end - start)) &&\n> +\t\t\t\tend=$start &&\n> +\t\t\t\techo \"$oid $size\" ||\n> +\t\t\t\treturn 1\n> +\t\t\tdone <idx.sorted ||\n>  \t\t\treturn 1\n>  \t\tdone\n>  \t} >expect.raw &&\n"},{"id":"486272","messageId":"xmqqa5pm9tnx.fsf@gitster.g","threadId":"60680","inReplyTo":"ZZWOtnP2IHNldcy6@nand.local","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-03T18:08:18Z","receivedAt":"2024-01-03T18:08:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> * tb/path-filter-fix (2023-10-18) 17 commits\n>>  - bloom: introduce `deinit_bloom_filters()`\n>>  ...\n>>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n>>\n>>  The Bloom filter used for path limited history traversal was broken\n>>  on systems whose \"char\" is unsigned; update the implementation and\n>>  bump the format version to 2.\n>>\n>>  Expecting a reroll.\n>>  cf. <20231023202212.GA5470@szeder.dev>\n>>  source: <cover.1697653929.git.me@ttaylorr.com>\n>\n> I was confused by this one, since I couldn't figure out which tests\n> Gábor was referring to here. I responded in [1], but haven't heard back\n> since the end of October.\n> ...\n> [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n\nOK, let's ping just once then.\n\nThanks.\n"},{"id":"486340","messageId":"20240105085928.GA3078702@coredump.intra.peff.net","threadId":"60680","inReplyTo":"xmqq8r56bcew.fsf@gitster.g","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-05T08:59:28Z","receivedAt":"2024-01-05T08:59:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 03, 2024 at 08:37:59AM -0800, Junio C Hamano wrote:\n\n> This is totally unrelated tangent, but the way \"show-index\" gets\n> invoked in the above loop makes readers wonder how the caller found\n> out which $idx file to read.\n> \n> Of course, the above loop sits downstream of a pipe\n> \n>     find .git/objects/pack -type f -name \\*.idx\n> \n> which means that any user of \"git show-index\" must be intimately\n> familiar with how the object database is structured.  I wonder if we\n> want an extra layer of abstraction, similar to how the reference\n> database can have different backend implementation.\n\nI assume you mean a test helper by \"extra layer of abstraction\".\n\nThat could make sense, though I think this is just the tip of the\niceberg. There are a bazillion tests that muck with .git/objects/\ndirectly (especially ones finding and munging loose objects). So it\nwouldn't do much good until we know what cases the abstract code would\nhave to handle. And I don't think we have any concrete alternative yet\nto the current object-dir layout.\n\nSo I think we should just punt on it for now. Adding one case here is\nnot making a hypothetical abstraction layer significantly harder to add\nlater.\n\n-Peff\n"},{"id":"486347","messageId":"xmqqo7dzoi2e.fsf@gitster.g","threadId":"60680","inReplyTo":"20240105085928.GA3078702@coredump.intra.peff.net","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-05T16:34:17Z","receivedAt":"2024-01-05T16:34:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 03, 2024 at 08:37:59AM -0800, Junio C Hamano wrote:\n>\n>> This is totally unrelated tangent, but the way \"show-index\" gets\n>> invoked in the above loop makes readers wonder how the caller found\n>> out which $idx file to read.\n>> \n>> Of course, the above loop sits downstream of a pipe\n>> \n>>     find .git/objects/pack -type f -name \\*.idx\n>> \n>> which means that any user of \"git show-index\" must be intimately\n>> familiar with how the object database is structured.  I wonder if we\n>> want an extra layer of abstraction, similar to how the reference\n>> database can have different backend implementation.\n>\n> I assume you mean a test helper by \"extra layer of abstraction\".\n\nWell, that might be a good first step, but I was envisioning more\ninto far future where the object store is reimplemented on top of\nhigh-performance database, at which point we may want subcommands\nlike \"git odb list-packs\" that lists packfile names (which may not\nname any on-filesystem files) that can be given to \"git show-index\",\nfor example.  Of course, the abstraction boundary of the object\nstore might be placed a lot higher and the interface into it may be\nlimited to \"here is an oid, give me its contents\" without distinction\nbetween the objects in packfiles and objects in loose object files.\n\n> So I think we should just punt on it for now. Adding one case here is\n> not making a hypothetical abstraction layer significantly harder to add\n> later.\n\nTotally agree, and that is why I labeled the whole thing as an\nunrelated tangent.\n\nThanks.\n"},{"id":"486764","messageId":"20240113183544.GA3000857@szeder.dev","threadId":"60680","inReplyTo":"xmqqa5pm9tnx.fsf@gitster.g","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-01-13T18:35:44Z","receivedAt":"2024-01-13T18:35:47Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> >> * tb/path-filter-fix (2023-10-18) 17 commits\n> >>  - bloom: introduce `deinit_bloom_filters()`\n> >>  ...\n> >>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n> >>\n> >>  The Bloom filter used for path limited history traversal was broken\n> >>  on systems whose \"char\" is unsigned; update the implementation and\n> >>  bump the format version to 2.\n> >>\n> >>  Expecting a reroll.\n> >>  cf. <20231023202212.GA5470@szeder.dev>\n> >>  source: <cover.1697653929.git.me@ttaylorr.com>\n> >\n> > I was confused by this one, since I couldn't figure out which tests\n> > Gábor was referring to here. I responded in [1], but haven't heard back\n> > since the end of October.\n> > ...\n> > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n\nI keep referring to the test in:\n\n  https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/\n\nwhich, rather disappointingly, is still the only test out there\nexercising the interaction between split commit graphs and different\nmodified path Bloom filter versions.  Note that in that message I\nmentioned that merging layers with differenet Bloom filter versions\nseemed to work, but that, alas, is no longer the case, because it's\nnow broken in Taylor's recent iterations of the patch series.\n\nAt the risk of sounding like a broken record: the interaction of split\ncommit graphs and different Bloom filter versions should be thoroughly\ntested.\n\n\n"},{"id":"486767","messageId":"ZaMJU6MJ5wZxyLeM@nand.local","threadId":"60680","inReplyTo":"20240113183544.GA3000857@szeder.dev","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-13T22:06:11Z","receivedAt":"2024-01-13T22:06:14Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Gábor,\n\nThanks very much for your patience while I figure all of this out. I'm\nconfident that with the fixup! below that your test passes in the next\nround of this series.\n\nOn Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote:\n> On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:\n> > Taylor Blau <me@ttaylorr.com> writes:\n> >\n> > >> * tb/path-filter-fix (2023-10-18) 17 commits\n> > >>  - bloom: introduce `deinit_bloom_filters()`\n> > >>  ...\n> > >>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n> > >>\n> > >>  The Bloom filter used for path limited history traversal was broken\n> > >>  on systems whose \"char\" is unsigned; update the implementation and\n> > >>  bump the format version to 2.\n> > >>\n> > >>  Expecting a reroll.\n> > >>  cf. <20231023202212.GA5470@szeder.dev>\n> > >>  source: <cover.1697653929.git.me@ttaylorr.com>\n> > >\n> > > I was confused by this one, since I couldn't figure out which tests\n> > > Gábor was referring to here. I responded in [1], but haven't heard back\n> > > since the end of October.\n> > > ...\n> > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n>\n> I keep referring to the test in:\n>\n>   https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/\n>\n> which, rather disappointingly, is still the only test out there\n> exercising the interaction between split commit graphs and different\n> modified path Bloom filter versions.  Note that in that message I\n> mentioned that merging layers with differenet Bloom filter versions\n> seemed to work, but that, alas, is no longer the case, because it's\n> now broken in Taylor's recent iterations of the patch series.\n\nThanks for clarifying. With all of the different versions of this series\nand the couple of tests that you and I were talking about, I think that\nI got confused and thought you were referring to a different test.\n\nIndeed, the current version of this series (v4) does not pass the test\nin <20230830200218.GA5147@szeder.dev>, but the fix in order for it to\npass is relatively straightforward.\n\nI have this on top of my local branch as a fixup! and I'll squash it\ndown on Tuesday[^1] and send a reroll after double-checking that the fix\nis correct and doesn't conflict with any other parts of the series.\n\nSince it's been so long since I've looked at this, I'll re-read the\nseries as a whole with fresh eyes to make sure that everything still\nmakes sense.\n\nIn any case, here's the patch on top (with a lightly modified version of\nthe test you wrote in <20230830200218.GA5147@szeder.dev>:\n\nSubject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with\n consistent settings\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c       |  3 ++-\n t/t4216-log-bloom.sh | 29 ++++++++++++++++++++++++++++-\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 60fa64d956..82a4632c4e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -507,7 +507,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g)\n \t\t}\n\n \t\tif (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry ||\n-\t\t    g->bloom_filter_settings->num_hashes != settings->num_hashes) {\n+\t\t    g->bloom_filter_settings->num_hashes != settings->num_hashes ||\n+\t\t    g->bloom_filter_settings->hash_version != settings->hash_version) {\n \t\t\tg->chunk_bloom_indexes = NULL;\n \t\t\tg->chunk_bloom_data = NULL;\n \t\t\tFREE_AND_NULL(g->bloom_filter_settings);\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 569f2b6f8b..d8c42e2e27 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -438,7 +438,7 @@ test_expect_success 'setup for mixed Bloom setting tests' '\n \tdone\n '\n\n-test_expect_success 'ensure incompatible Bloom filters are ignored' '\n+test_expect_success 'ensure Bloom filters with incompatible settings are ignored' '\n \t# Compute Bloom filters with \"unusual\" settings.\n \tgit -C $repo rev-parse one >in &&\n \tGIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \\\n@@ -488,6 +488,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '\n \ttest_must_be_empty err\n '\n\n+test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '\n+\trm \"$repo/$graph\" &&\n+\n+\tgit -C $repo log --oneline --no-decorate -- $CENT >expect &&\n+\n+\t# Compute v1 Bloom filters for commits at the bottom.\n+\tgit -C $repo rev-parse HEAD^ >in &&\n+\tgit -C $repo commit-graph write --stdin-commits --changed-paths \\\n+\t\t--split <in &&\n+\n+\t# Compute v2 Bloomfilters for the rest of the commits at the top.\n+\tgit -C $repo rev-parse HEAD >in &&\n+\tgit -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \\\n+\t\t--stdin-commits --changed-paths --split=no-merge <in &&\n+\n+\ttest_line_count = 2 $repo/$chain &&\n+\n+\tgit -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\n+\tlayer=\"$(head -n 1 $repo/$chain)\" &&\n+\tcat >expect.err <<-EOF &&\n+\twarning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n get_first_changed_path_filter () {\n \ttest-tool read-graph bloom-filters >filters.dat &&\n \thead -n 1 filters.dat\n\n--\n2.38.0.16.g393fd4c6db\n\n(Excuse the old version of Git here, I'm in the middle of a long-running\nprocess which checks out older versions of the tree to count the number\nof unused-parameters over time.)\n\nThanks,\nTaylor\n\n[^1]: Monday is a US holiday, so I likely won't be at my computer.\n"},{"id":"486769","messageId":"20240113225157.GD3000857@szeder.dev","threadId":"60680","inReplyTo":"20240113183544.GA3000857@szeder.dev","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-01-13T22:51:57Z","receivedAt":"2024-01-13T22:52:00Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote:\n> On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:\n> > Taylor Blau <me@ttaylorr.com> writes:\n> > \n> > >> * tb/path-filter-fix (2023-10-18) 17 commits\n> > >>  - bloom: introduce `deinit_bloom_filters()`\n> > >>  ...\n> > >>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n> > >>\n> > >>  The Bloom filter used for path limited history traversal was broken\n> > >>  on systems whose \"char\" is unsigned; update the implementation and\n> > >>  bump the format version to 2.\n> > >>\n> > >>  Expecting a reroll.\n> > >>  cf. <20231023202212.GA5470@szeder.dev>\n> > >>  source: <cover.1697653929.git.me@ttaylorr.com>\n> > >\n> > > I was confused by this one, since I couldn't figure out which tests\n> > > Gábor was referring to here. I responded in [1], but haven't heard back\n> > > since the end of October.\n> > > ...\n> > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n> \n> I keep referring to the test in:\n> \n>   https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/\n> \n> which, rather disappointingly, is still the only test out there\n> exercising the interaction between split commit graphs and different\n> modified path Bloom filter versions.  Note that in that message I\n> mentioned that merging layers with differenet Bloom filter versions\n> seemed to work, but that, alas, is no longer the case, because it's\n> now broken in Taylor's recent iterations of the patch series.\n> \n> At the risk of sounding like a broken record: the interaction of split\n> commit graphs and different Bloom filter versions should be thoroughly\n> tested.\n\nOn a related note, if current git (I tried current master and v2.43.0)\nencounters a commit graph layer containing v2 Bloom filters (created\nby current seen) while writing a new commit graph, then it segfaults\ndereferencing a NULL 'settings' pointer in\nget_or_compute_bloom_filter().\n\nThe test below demonstrates this, but it's quite hacky using two\ndifferent git versions: it has to be run by an old git version not yet\nsupporting v2 Bloom filters, and a new git version already supporting\nthem should be installed at /tmp/git-new/.\n\n\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 2ba0324a69..0464dd68d5 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -454,4 +454,33 @@ test_expect_success 'Bloom reader notices out-of-order index offsets' '\n \ttest_cmp expect.err err\n '\n \n+CENT=$(printf \"\\302\\242\")\n+test_expect_success 'split commit graph vs changed paths Bloom filter v2 vs old git' '\n+\tgit init split-v2-old &&\n+\t(\n+\t\tcd split-v2-old &&\n+\t\tgit commit --allow-empty -m \"Bloom filters are written but still ignored for root commits :(\" &&\n+\t\tfor i in 1 2 3\n+\t\tdo\n+\t\t\techo $i >$CENT &&\n+\t\t\tgit add $CENT &&\n+\t\t\tgit commit -m \"$i\" || return 1\n+\t\tdone &&\n+\t\tgit log --oneline -- $CENT >expect &&\n+\n+\t\t# Here we write a commit graph layer containing v2 changed\n+\t\t# path Bloom filters using a git binary built from current\n+\t\t# 'seen' branch.\n+\t\tgit rev-parse HEAD^ |\n+\t\t/tmp/git-new/bin/git -c commitgraph.changedPathsVersion=2 \\\n+\t\t\tcommit-graph write --stdin-commits --changed-paths --split &&\n+\n+\t\t# This is current master, and segfaults.\n+\t\tgit commit-graph write --reachable --changed-paths &&\n+\n+\t\tgit log --oneline -- $CENT >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n\n\n(gdb) r\nStarting program: /home/szeder/src/git/git commit-graph write --reachable --changed-paths\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n\nProgram received signal SIGSEGV, Segmentation fault.\n0x00005555556b8714 in get_or_compute_bloom_filter (r=0x555555a0e5a0 <the_repo>, c=0x555555a1cdd0, compute_if_not_present=1, settings=0x0, computed=0x7fffffffd634) at bloom.c:253\n253\t\tdiffopt.max_changes = settings->max_changed_paths;\n(gdb) l\n248\t\t\treturn NULL;\n249\t\n250\t\trepo_diff_setup(r, &diffopt);\n251\t\tdiffopt.flags.recursive = 1;\n252\t\tdiffopt.detect_rename = 0;\n253\t\tdiffopt.max_changes = settings->max_changed_paths;\n254\t\tdiff_setup_done(&diffopt);\n255\t\n256\t\t/* ensure commit is parsed so we have parent information */\n257\t\trepo_parse_commit(r, c);\n(gdb) bt\n#0  0x00005555556b8714 in get_or_compute_bloom_filter (\n    r=0x555555a0e5a0 <the_repo>, c=0x555555a1cdd0, compute_if_not_present=1, \n    settings=0x0, computed=0x7fffffffd634) at bloom.c:253\n#1  0x00005555556d16d5 in compute_bloom_filters (ctx=0x555555a1c200)\n    at commit-graph.c:1783\n#2  0x00005555556d4329 in write_commit_graph (odb=0x555555a19210, \n    pack_indexes=0x0, commits=0x7fffffffd7c0, \n    flags=(COMMIT_GRAPH_WRITE_PROGRESS | COMMIT_GRAPH_WRITE_BLOOM_FILTERS), \n    opts=0x5555559ef8d0 <write_opts>) at commit-graph.c:2603\n#3  0x00005555556d19ab in write_commit_graph_reachable (odb=0x555555a19210, \n    flags=(COMMIT_GRAPH_WRITE_PROGRESS | COMMIT_GRAPH_WRITE_BLOOM_FILTERS), \n    opts=0x5555559ef8d0 <write_opts>) at commit-graph.c:1849\n#4  0x00005555555a63c2 in graph_write (argc=0, argv=0x7fffffffde20, prefix=0x0)\n    at builtin/commit-graph.c:294\n#5  0x00005555555a66f4 in cmd_commit_graph (argc=3, argv=0x7fffffffde20, \n    prefix=0x0) at builtin/commit-graph.c:353\n#6  0x0000555555575b43 in run_builtin (p=0x5555559da260 <commands+576>, \n    argc=4, argv=0x7fffffffde20) at git.c:469\n#7  0x0000555555575fcb in handle_builtin (argc=4, argv=0x7fffffffde20)\n    at git.c:724\n#8  0x0000555555576272 in run_argv (argcp=0x7fffffffdc7c, argv=0x7fffffffdc70)\n    at git.c:788\n#9  0x000055555557685d in cmd_main (argc=4, argv=0x7fffffffde20) at git.c:923\n#10 0x000055555568ad55 in main (argc=5, argv=0x7fffffffde18)\n    at common-main.c:62\n\n\n"},{"id":"486770","messageId":"20240113234134.GE3000857@szeder.dev","threadId":"60680","inReplyTo":"ZaMJU6MJ5wZxyLeM@nand.local","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-01-13T23:41:34Z","receivedAt":"2024-01-13T23:41:37Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Jan 13, 2024 at 05:06:11PM -0500, Taylor Blau wrote:\n> Hi Gábor,\n> \n> Thanks very much for your patience while I figure all of this out. I'm\n> confident that with the fixup! below that your test passes in the next\n> round of this series.\n> \n> On Sat, Jan 13, 2024 at 07:35:44PM +0100, SZEDER Gábor wrote:\n> > On Wed, Jan 03, 2024 at 10:08:18AM -0800, Junio C Hamano wrote:\n> > > Taylor Blau <me@ttaylorr.com> writes:\n> > >\n> > > >> * tb/path-filter-fix (2023-10-18) 17 commits\n> > > >>  - bloom: introduce `deinit_bloom_filters()`\n> > > >>  ...\n> > > >>  - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`\n> > > >>\n> > > >>  The Bloom filter used for path limited history traversal was broken\n> > > >>  on systems whose \"char\" is unsigned; update the implementation and\n> > > >>  bump the format version to 2.\n> > > >>\n> > > >>  Expecting a reroll.\n> > > >>  cf. <20231023202212.GA5470@szeder.dev>\n> > > >>  source: <cover.1697653929.git.me@ttaylorr.com>\n> > > >\n> > > > I was confused by this one, since I couldn't figure out which tests\n> > > > Gábor was referring to here. I responded in [1], but haven't heard back\n> > > > since the end of October.\n> > > > ...\n> > > > [1]: https://lore.kernel.org/git/ZUARCJ1MmqgXfS4i@nand.local/\n> >\n> > I keep referring to the test in:\n> >\n> >   https://public-inbox.org/git/20230830200218.GA5147@szeder.dev/\n> >\n> > which, rather disappointingly, is still the only test out there\n> > exercising the interaction between split commit graphs and different\n> > modified path Bloom filter versions.  Note that in that message I\n> > mentioned that merging layers with differenet Bloom filter versions\n> > seemed to work, but that, alas, is no longer the case, because it's\n> > now broken in Taylor's recent iterations of the patch series.\n> \n> Thanks for clarifying. With all of the different versions of this series\n> and the couple of tests that you and I were talking about, I think that\n> I got confused and thought you were referring to a different test.\n> \n> Indeed, the current version of this series (v4) does not pass the test\n> in <20230830200218.GA5147@szeder.dev>, but the fix in order for it to\n> pass is relatively straightforward.\n> \n> I have this on top of my local branch as a fixup! and I'll squash it\n> down on Tuesday[^1] and send a reroll after double-checking that the fix\n> is correct and doesn't conflict with any other parts of the series.\n> \n> Since it's been so long since I've looked at this, I'll re-read the\n> series as a whole with fresh eyes to make sure that everything still\n> makes sense.\n> \n> In any case, here's the patch on top (with a lightly modified version of\n> the test you wrote in <20230830200218.GA5147@szeder.dev>:\n\nI certainly hope that I'm just misunderstanding, and you don't\nactually imply that this one test in its current form would qualify as\nthorough testing... :)\n\n\n> Subject: [PATCH] fixup! commit-graph: ensure Bloom filters are read with\n>  consistent settings\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  commit-graph.c       |  3 ++-\n>  t/t4216-log-bloom.sh | 29 ++++++++++++++++++++++++++++-\n>  2 files changed, 30 insertions(+), 2 deletions(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 60fa64d956..82a4632c4e 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -507,7 +507,8 @@ static void validate_mixed_bloom_settings(struct commit_graph *g)\n>  \t\t}\n> \n>  \t\tif (g->bloom_filter_settings->bits_per_entry != settings->bits_per_entry ||\n> -\t\t    g->bloom_filter_settings->num_hashes != settings->num_hashes) {\n> +\t\t    g->bloom_filter_settings->num_hashes != settings->num_hashes ||\n> +\t\t    g->bloom_filter_settings->hash_version != settings->hash_version) {\n>  \t\t\tg->chunk_bloom_indexes = NULL;\n>  \t\t\tg->chunk_bloom_data = NULL;\n>  \t\t\tFREE_AND_NULL(g->bloom_filter_settings);\n> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\n> index 569f2b6f8b..d8c42e2e27 100755\n> --- a/t/t4216-log-bloom.sh\n> +++ b/t/t4216-log-bloom.sh\n> @@ -438,7 +438,7 @@ test_expect_success 'setup for mixed Bloom setting tests' '\n>  \tdone\n>  '\n> \n> -test_expect_success 'ensure incompatible Bloom filters are ignored' '\n> +test_expect_success 'ensure Bloom filters with incompatible settings are ignored' '\n>  \t# Compute Bloom filters with \"unusual\" settings.\n>  \tgit -C $repo rev-parse one >in &&\n>  \tGIT_TEST_BLOOM_SETTINGS_NUM_HASHES=3 git -C $repo commit-graph write \\\n> @@ -488,6 +488,33 @@ test_expect_success 'merge graph layers with incompatible Bloom settings' '\n>  \ttest_must_be_empty err\n>  '\n> \n> +test_expect_success 'ensure Bloom filter with incompatible versions are ignored' '\n> +\trm \"$repo/$graph\" &&\n> +\n> +\tgit -C $repo log --oneline --no-decorate -- $CENT >expect &&\n> +\n> +\t# Compute v1 Bloom filters for commits at the bottom.\n> +\tgit -C $repo rev-parse HEAD^ >in &&\n> +\tgit -C $repo commit-graph write --stdin-commits --changed-paths \\\n> +\t\t--split <in &&\n> +\n> +\t# Compute v2 Bloomfilters for the rest of the commits at the top.\n> +\tgit -C $repo rev-parse HEAD >in &&\n> +\tgit -C $repo -c commitGraph.changedPathsVersion=2 commit-graph write \\\n> +\t\t--stdin-commits --changed-paths --split=no-merge <in &&\n> +\n> +\ttest_line_count = 2 $repo/$chain &&\n> +\n> +\tgit -C $repo log --oneline --no-decorate -- $CENT >actual 2>err &&\n> +\ttest_cmp expect actual &&\n> +\n> +\tlayer=\"$(head -n 1 $repo/$chain)\" &&\n> +\tcat >expect.err <<-EOF &&\n> +\twarning: disabling Bloom filters for commit-graph layer $SQ$layer$SQ due to incompatible settings\n> +\tEOF\n> +\ttest_cmp expect.err err\n> +'\n> +\n>  get_first_changed_path_filter () {\n>  \ttest-tool read-graph bloom-filters >filters.dat &&\n>  \thead -n 1 filters.dat\n> \n> --\n> 2.38.0.16.g393fd4c6db\n> \n> (Excuse the old version of Git here, I'm in the middle of a long-running\n> process which checks out older versions of the tree to count the number\n> of unused-parameters over time.)\n> \n> Thanks,\n> Taylor\n> \n> [^1]: Monday is a US holiday, so I likely won't be at my computer.\n"},{"id":"486864","messageId":"ZabpBHBB1TXIXJLr@nand.local","threadId":"60680","inReplyTo":"20240113234134.GE3000857@szeder.dev","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T20:37:24Z","receivedAt":"2024-01-16T20:37:26Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Gábor,\n\nOn Sun, Jan 14, 2024 at 12:41:34AM +0100, SZEDER Gábor wrote:\n> > In any case, here's the patch on top (with a lightly modified version of\n> > the test you wrote in <20230830200218.GA5147@szeder.dev>:\n>\n> I certainly hope that I'm just misunderstanding, and you don't\n> actually imply that this one test in its current form would qualify as\n> thorough testing... :)\n\nI hear what you're saying, though I think that the interesting behavior\nthat would be most likely to regress is the transition between different\nBloom filter settings/hash-version across split commit-graph layers.\n\nWe have extensive tests on either \"side\" of this transition for both v1\nand v2 Bloom filters, so I'm not sure what we'd want to add there. Like\nI said, the transition is the primary (previously-)untested area of this\ncode that I would want to ensure is covered to protect against\nregressions.\n\nI think that the most recent round of this series accomplishes that\ngoal.\n\nThanks,\nTaylor\n"},{"id":"486866","messageId":"Zabr1Glljjgl/UUB@nand.local","threadId":"60680","inReplyTo":"20240113225157.GD3000857@szeder.dev","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T20:49:24Z","receivedAt":"2024-01-16T20:49:26Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Jan 13, 2024 at 11:51:57PM +0100, SZEDER Gábor wrote:\n> On a related note, if current git (I tried current master and v2.43.0)\n> encounters a commit graph layer containing v2 Bloom filters (created\n> by current seen) while writing a new commit graph, then it segfaults\n> dereferencing a NULL 'settings' pointer in\n> get_or_compute_bloom_filter().\n>\n> The test below demonstrates this, but it's quite hacky using two\n> different git versions: it has to be run by an old git version not yet\n> supporting v2 Bloom filters, and a new git version already supporting\n> them should be installed at /tmp/git-new/.\n>\n> diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\n> index 2ba0324a69..0464dd68d5 100755\n> --- a/t/t4216-log-bloom.sh\n> +++ b/t/t4216-log-bloom.sh\n> @@ -454,4 +454,33 @@ test_expect_success 'Bloom reader notices out-of-order index offsets' '\n>  \ttest_cmp expect.err err\n>  '\n>\n> +CENT=$(printf \"\\302\\242\")\n> +test_expect_success 'split commit graph vs changed paths Bloom filter v2 vs old git' '\n> +\tgit init split-v2-old &&\n> +\t(\n> +\t\tcd split-v2-old &&\n> +\t\tgit commit --allow-empty -m \"Bloom filters are written but still ignored for root commits :(\" &&\n> +\t\tfor i in 1 2 3\n> +\t\tdo\n> +\t\t\techo $i >$CENT &&\n> +\t\t\tgit add $CENT &&\n> +\t\t\tgit commit -m \"$i\" || return 1\n> +\t\tdone &&\n> +\t\tgit log --oneline -- $CENT >expect &&\n> +\n> +\t\t# Here we write a commit graph layer containing v2 changed\n> +\t\t# path Bloom filters using a git binary built from current\n> +\t\t# 'seen' branch.\n> +\t\tgit rev-parse HEAD^ |\n> +\t\t/tmp/git-new/bin/git -c commitgraph.changedPathsVersion=2 \\\n> +\t\t\tcommit-graph write --stdin-commits --changed-paths --split &&\n> +\n> +\t\t# This is current master, and segfaults.\n> +\t\tgit commit-graph write --reachable --changed-paths &&\n> +\n> +\t\tgit log --oneline -- $CENT >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_done\n\nThanks. The segfault is reproducible on my end, but I don't think that\nit is possible to fix this for existing versions of Git. The problem (as\nyou note in your backtrace) is here:\n\n    #0  0x000055555569c842 in get_or_compute_bloom_filter (\n        r=0x5555559c9ce0 <the_repo>, c=0x5555559dffd0, compute_if_not_present=1,\n        settings=0x0, computed=0x7fffffffe0f4) at bloom.c:253\n\nWhich tries to dereference ctx->bloom_settings, which is NULL. Note that\nwe initialize some sensible defaults for ctx->bloom_settings in\ncommit-graph.c::write_commit_graph():\n\n    struct bloom_filter_settings bloom_settings = DEFAULT_BLOOM_FILTER_SETTINGS;\n    /* ... */\n    bloom_settings.bits_per_entry = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_BITS_PER_ENTRY\",\n                                                  bloom_settings.bits_per_entry);\n    bloom_settings.num_hashes = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_NUM_HASHES\",\n                                              bloom_settings.num_hashes);\n    bloom_settings.max_changed_paths = git_env_ulong(\"GIT_TEST_BLOOM_SETTINGS_MAX_CHANGED_PATHS\",\n                                                     bloom_settings.max_changed_paths);\n    ctx->bloom_settings = &bloom_settings;\n\n...but we'll throw those away in favor of whatever is in the topmost\nlayer of the existing commit-graph chain later on in that same function:\n\n    if (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {\n      struct commit_graph *g;\n\n      g = ctx->r->objects->commit_graph;\n\n      /* We have changed-paths already. Keep them in the next graph */\n      if (g && g->chunk_bloom_data) {\n        ctx->changed_paths = 1;\n        ctx->bloom_settings = g->bloom_filter_settings;\n      }\n    }\n\nOK, everything seems fine thus far, until we inspect the value of\ng->bloom_filter_settings, which is NULL, becuase of this hunk from\ncommit-graph.c::graph_read_bloom_data():\n\n    if (hash_version != 1)\n      return 0;\n\nwhich terminates the function before we assign g->bloom_filter_settings\nfor the existing (written with v2 Bloom filters) graph layer.\n\nI don't think that there is a way to fix this in a backwards compatible\nway, but I'm comfortable with that in this instance since we don't\nexpect users to upgrading to v2 Bloom filters and then writing new graph\nlayers using a non-v2 compatible version of Git.\n\nWe can add a warning in the series that I'm working on indicating this,\nbut I don't think there's much more we can do besides changing this to\nindicate a warning and bailing instead of segfaulting.\n\nThanks,\nTaylor\n"},{"id":"486896","messageId":"xmqqfrywyk16.fsf@gitster.g","threadId":"60680","inReplyTo":"Zabr1Glljjgl/UUB@nand.local","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T22:45:41Z","receivedAt":"2024-01-16T22:45:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> OK, everything seems fine thus far, until we inspect the value of\n> g->bloom_filter_settings, which is NULL, becuase of this hunk from\n> commit-graph.c::graph_read_bloom_data():\n>\n>     if (hash_version != 1)\n>       return 0;\n>\n> which terminates the function before we assign g->bloom_filter_settings\n> for the existing (written with v2 Bloom filters) graph layer.\n>\n> I don't think that there is a way to fix this in a backwards compatible\n> way, but I'm comfortable with that in this instance since we don't\n> expect users to upgrading to v2 Bloom filters and then writing new graph\n> layers using a non-v2 compatible version of Git.\n\nA big red button solution to avoid this would be to uprev the\nrepository format version once you start writing v2 Bloom filters\nanywhere in the layers.  That way, existing Git clients would not be\nable to touch it.  I do not know if the cure is more severe than the\ndisease in that case, though.\n\nIn any case, at least, we should be able to prepare the code that we\nteach to grok v2 today so that they do not trigger the same segfault\nwhen they see a commit graph layer containing v3 Bloom filters (or\nlater).  Then we won't have to have the same conversation when we\nsomehow need to update Bloom filters again.\n\n\n\n"},{"id":"486900","messageId":"ZacRx1rbESvYiVgN@nand.local","threadId":"60680","inReplyTo":"xmqqfrywyk16.fsf@gitster.g","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T23:31:19Z","receivedAt":"2024-01-16T23:31:21Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 16, 2024 at 02:45:41PM -0800, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > OK, everything seems fine thus far, until we inspect the value of\n> > g->bloom_filter_settings, which is NULL, becuase of this hunk from\n> > commit-graph.c::graph_read_bloom_data():\n> >\n> >     if (hash_version != 1)\n> >       return 0;\n> >\n> > which terminates the function before we assign g->bloom_filter_settings\n> > for the existing (written with v2 Bloom filters) graph layer.\n> >\n> > I don't think that there is a way to fix this in a backwards compatible\n> > way, but I'm comfortable with that in this instance since we don't\n> > expect users to upgrading to v2 Bloom filters and then writing new graph\n> > layers using a non-v2 compatible version of Git.\n>\n> A big red button solution to avoid this would be to uprev the\n> repository format version once you start writing v2 Bloom filters\n> anywhere in the layers.  That way, existing Git clients would not be\n> able to touch it.  I do not know if the cure is more severe than the\n> disease in that case, though.\n\nI tend to think that in this case the cure is probably worse than the\ndisease. I expect it to be extremely rare that a user would upgrade to a\nmodern version of Git, write commit-graphs, then downgrade, and try to\nwrite more commit-graphs.\n\n> In any case, at least, we should be able to prepare the code that we\n> teach to grok v2 today so that they do not trigger the same segfault\n> when they see a commit graph layer containing v3 Bloom filters (or\n> later).  Then we won't have to have the same conversation when we\n> somehow need to update Bloom filters again.\n\nThis series should accomplish that by loading the Bloom chunk\nunconditionally, and only reading its filters when they match the\ngiven hash_version.\n\nThanks,\nTaylor\n"},{"id":"486902","messageId":"xmqqedegx2tt.fsf@gitster.g","threadId":"60680","inReplyTo":"ZacRx1rbESvYiVgN@nand.local","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-16T23:42:38Z","receivedAt":"2024-01-16T23:42:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> A big red button solution to avoid this would be to uprev the\n>> repository format version once you start writing v2 Bloom filters\n>> anywhere in the layers.  That way, existing Git clients would not be\n>> able to touch it.  I do not know if the cure is more severe than the\n>> disease in that case, though.\n>\n> I tend to think that in this case the cure is probably worse than the\n> disease. I expect it to be extremely rare that a user would upgrade to a\n> modern version of Git, write commit-graphs, then downgrade, and try to\n> write more commit-graphs.\n\nBut then the big red button solution would rarely misfire for users\nbecause they will not downgrade (and see \"gee, I now need to stick\nto the newer version\"), no?\n\nI am not seriously suggesting to do this, but I am not sure if\ndocumenting \"don't do this because you'll break your repository\"\nis sufficient.\n\n>> In any case, at least, we should be able to prepare the code that we\n>> teach to grok v2 today so that they do not trigger the same segfault\n>> when they see a commit graph layer containing v3 Bloom filters (or\n>> later).  Then we won't have to have the same conversation when we\n>> somehow need to update Bloom filters again.\n>\n> This series should accomplish that by loading the Bloom chunk\n> unconditionally, and only reading its filters when they match the\n> given hash_version.\n\nGood.\n"},{"id":"489336","messageId":"20240225225918.GB1940392@szeder.dev","threadId":"60680","inReplyTo":"ZabpBHBB1TXIXJLr@nand.local","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-02-25T22:59:18Z","receivedAt":"2024-02-25T22:59:21Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jan 16, 2024 at 03:37:24PM -0500, Taylor Blau wrote:\n> Hi Gábor,\n> \n> On Sun, Jan 14, 2024 at 12:41:34AM +0100, SZEDER Gábor wrote:\n> > > In any case, here's the patch on top (with a lightly modified version of\n> > > the test you wrote in <20230830200218.GA5147@szeder.dev>:\n> >\n> > I certainly hope that I'm just misunderstanding, and you don't\n> > actually imply that this one test in its current form would qualify as\n> > thorough testing... :)\n> \n> I hear what you're saying, though I think that the interesting behavior\n> that would be most likely to regress is the transition between different\n> Bloom filter settings/hash-version across split commit-graph layers.\n> \n> We have extensive tests on either \"side\" of this transition for both v1\n> and v2 Bloom filters, so I'm not sure what we'd want to add there. Like\n> I said, the transition is the primary (previously-)untested area of this\n> code that I would want to ensure is covered to protect against\n> regressions.\n> \n> I think that the most recent round of this series accomplishes that\n> goal.\n\nIt's great that we finally have test cases for different Bloom filter\nsettings in different commit-graph layers, including a test case that\nmerges those layers, but that test case doesn't check that the\nresulting merged commit-graph file contains the right settings.  And\nthere is still no test case that merges layers with different Bloom\nfilter versions.\nI think adding these would be the bare minimum...  and would need more\nfor due diligence.\n\n"},{"id":"489376","messageId":"Zdyj5RsW59b5kC7m@nand.local","threadId":"60680","inReplyTo":"20240225225918.GB1940392@szeder.dev","subject":"Re: What's cooking in git.git (Jan 2024, #01; Tue, 2)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-26T14:44:53Z","receivedAt":"2024-02-26T14:44:55Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi SZEDER,\n\nOn Sun, Feb 25, 2024 at 11:59:18PM +0100, SZEDER Gábor wrote:\n> It's great that we finally have test cases for different Bloom filter\n> settings in different commit-graph layers, including a test case that\n> merges those layers, but that test case doesn't check that the\n> resulting merged commit-graph file contains the right settings.  And\n> there is still no test case that merges layers with different Bloom\n> filter versions.\n\nThanks for reviewing.\n\nI'm happy to produce another round of this series that would address\nwhat you've added here.\n\nAre there any other specific things you'd like to see addressed in a\nsubsequent round? I want to make sure that I'm addressing all of your\nconcerns, and avoid the need for yet another round of this series that\naddresses things that I could have done in the first place.\n\n> I think adding these would be the bare minimum...  and would need more\n> for due diligence.\n\nAs I said above, I'm happy to add these things in, but please do let me\nknow if there are others.\n\nThanks,\nTaylor\n"}]}