threads / discuss / 56785

What's cooking in git.git (Oct 2021, #06; Mon, 25)

Subject: What's cooking in git.git (Oct 2021, #06; Mon, 25)

## tl;dr

32 messages between Oct 26, 2021 and Nov 11, 2021.

replies: 31people: 11as markdown or json

Junio C Hamano· Oct 26, 2021, 03:48 UTC · lore

Here are the topics that have been cooking in my tree. Commits prefixed with '+' are in 'next' (being in 'next' is a sign that a topic is stable enough to be used and are candidate to be in a future release). Commits prefixed with '-' are only in 'seen', which means nothing more than that I have found them of interest for some reason (like "it may have hard-to-resolve conflicts with another topic already in flight" or "this may turn out to be useful"). Do not read too much into a topic being in (or not in) 'seen'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them.

The fifteenth batch of topics are in 'master'. I expect that this is more-or-less what we can expect in the -rc0, unless there is a hotfix to what's already merged.

Copies of the source code to Git live in many repositories, and the following is a list of the ones I push into or their mirrors. Some repositories have only a subset of branches.

With maint, master, next, seen, todo:
	git://git.kernel.org/pub/scm/git/git.git/
	git://repo.or.cz/alt-git.git/
	https://kernel.googlesource.com/pub/scm/git/git/
	https://github.com/git/git/
	https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
	https://github.com/gitster/git/

Even though the preformatted documentation in HTML and man format are not sources, they are published in these repositories for convenience (replace "htmldocs" with "manpages" for the manual pages):

	git://git.kernel.org/pub/scm/git/git-htmldocs.git/
	https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
	https://www.kernel.org/pub/software/scm/git/

-------------------------------------------------- [Graduated to 'master']

* ab/fix-commit-error-message-upon-unwritable-object-store (2021-10-12) 2 commits
  (merged to 'next' on 2021-10-14 at 08c52f5cd5)
 + commit: fix duplication regression in permission error output
 + unwritable tests: assert exact error output
 "git commit" gave duplicated error message when the object store
 was unwritable, which has been corrected.
* ab/fix-make-lint-docs (2021-10-15) 4 commits
  (merged to 'next' on 2021-10-18 at 22ebb3213f)
 + doc lint: make "lint-docs" non-.PHONY
 + doc build: speed up "make lint-docs"
 + doc lint: emit errors on STDERR
 + doc lint: fix error-hiding regression
 Build fix.
* ab/fsck-unexpected-type (2021-10-01) 17 commits
  (merged to 'next' on 2021-10-14 at 10edc78c9c)
 + fsck: report invalid object type-path combinations
 + fsck: don't hard die on invalid object types
 + object-file.c: stop dying in parse_loose_header()
 + object-file.c: return ULHR_TOO_LONG on "header too long"
 + object-file.c: use "enum" return type for unpack_loose_header()
 + object-file.c: simplify unpack_loose_short_header()
 + object-file.c: make parse_loose_header_extended() public
 + object-file.c: return -1, not "status" from unpack_loose_header()
 + object-file.c: don't set "typep" when returning non-zero
 + cat-file tests: test for current --allow-unknown-type behavior
 + cat-file tests: add corrupt loose object test
 + cat-file tests: test for missing/bogus object with -t, -s and -p
 + cat-file tests: move bogus_* variable declarations earlier
 + fsck tests: test for garbage appended to a loose object
 + fsck tests: test current hash/type mismatch behavior
 + fsck tests: refactor one test to use a sub-repo
 + fsck tests: add test for fsck-ing an unknown type
 "git fsck" has been taught to report mismatch between expected and
 actual types of an object better.
* ab/mark-leak-free-tests (2021-10-12) 10 commits
  (merged to 'next' on 2021-10-18 at c522807d5d)
 + leak tests: mark some misc tests as passing with SANITIZE=leak
 + leak tests: mark various "generic" tests as passing with SANITIZE=leak
 + leak tests: mark some read-tree tests as passing with SANITIZE=leak
 + leak tests: mark some ls-files tests as passing with SANITIZE=leak
 + leak tests: mark all checkout-index tests as passing with SANITIZE=leak
 + leak tests: mark all trace2 tests as passing with SANITIZE=leak
 + leak tests: mark all ls-tree tests as passing with SANITIZE=leak
 + leak tests: run various "test-tool" tests in t00*.sh SANITIZE=leak
 + leak tests: run various built-in tests in t00*.sh SANITIZE=leak
 + Merge branch 'ab/sanitize-leak-ci' into ab/mark-leak-free-tests
 Bunch of tests are marked as "passing leak check".
* ab/mark-leak-free-tests-more (2021-10-07) 8 commits
  (merged to 'next' on 2021-10-18 at fe798f77b8)
 + merge: add missing strbuf_release()
 + ls-files: add missing string_list_clear()
 + ls-files: fix a trivial dir_clear() leak
 + tests: fix test-oid-array leak, test in SANITIZE=leak
 + tests: fix a memory leak in test-oidtree.c
 + tests: fix a memory leak in test-parse-options.c
 + tests: fix a memory leak in test-prio-queue.c
 + Merge branch 'ab/sanitize-leak-ci' into ab/mark-leak-free-tests-more
 Bunch of tests are marked as "passing leak check".
* ab/parse-options-cleanup (2021-10-08) 10 commits
  (merged to 'next' on 2021-10-18 at 305d339ad9)
 + parse-options: change OPT_{SHORT,UNSET} to an enum
 + parse-options tests: test optname() output
 + parse-options.[ch]: make opt{bug,name}() "static"
 + commit-graph: stop using optname()
 + parse-options.c: move optname() earlier in the file
 + parse-options.h: make the "flags" in "struct option" an enum
 + parse-options.c: use exhaustive "case" arms for "enum parse_opt_result"
 + parse-options.[ch]: consistently use "enum parse_opt_result"
 + parse-options.[ch]: consistently use "enum parse_opt_flags"
 + parse-options.h: move PARSE_OPT_SHELL_EVAL between enums
 Random changes to parse-options implementation.
* ab/pkt-line-cleanup (2021-10-15) 2 commits
  (merged to 'next' on 2021-10-18 at 79b07663da)
 + pkt-line.[ch]: remove unused packet_read_line_buf()
 + pkt-line.[ch]: remove unused packet_buf_write_len()
 Code clean-up.
* ab/test-cleanly-recreate-trash-directory (2021-10-15) 1 commit
  (merged to 'next' on 2021-10-18 at 6fdb43973b)
 + test-lib.sh: try to re-chmod & retry on failed trash removal
 Improve test framework around unwritable directories.
* ab/test-lib-diff-cleanup (2021-10-15) 2 commits
  (merged to 'next' on 2021-10-18 at 5229c5d01d)
 + tests: stop using top-level "README" and "COPYING" files
 + "lib-diff" tests: make "README" and "COPYING" test data smaller
 Test clean-up.
* ab/unpack-trees-leakfix (2021-10-13) 4 commits
  (merged to 'next' on 2021-10-14 at bb54827704)
 + sequencer: fix a memory leak in do_reset()
 + sequencer: add a "goto cleanup" to do_reset()
 + unpack-trees: don't leak memory in verify_clean_subdirectory()
 + Merge branch 'ab/sanitize-leak-ci' into ab/unpack-trees-leakfix
 Leakfix.
* da/mergetools-special-case-xxdiff-exit-128 (2021-10-13) 1 commit
  (merged to 'next' on 2021-10-18 at 0dd8a08c63)
 + mergetools/xxdiff: prevent segfaults from stopping difftool
 The xxdiff difftool backend can exit with status 128, which the
 difftool-helper that launches the backend takes as a significant
 failure, when it is not significant at all.  Work it around.
* fs/ssh-signing (2021-09-10) 9 commits
  (merged to 'next' on 2021-10-11 at b456b95672)
 + ssh signing: test that gpg fails for unknown keys
 + ssh signing: tests for logs, tags & push certs
 + ssh signing: duplicate t7510 tests for commits
 + ssh signing: verify signatures using ssh-keygen
 + ssh signing: provide a textual signing_key_id
 + ssh signing: retrieve a default key from ssh-agent
 + ssh signing: add ssh key format and signing code
 + ssh signing: add test prereqs
 + ssh signing: preliminary refactoring and clean-up
 (this branch is used by fs/ssh-signing-fix and fs/ssh-signing-key-lifetime.)
 Use ssh public crypto for object and push-cert signing.
* fs/ssh-signing-fix (2021-10-18) 5 commits
  (merged to 'next' on 2021-10-18 at 5ffa706433)
 + gpg-interface: fix leak of strbufs in get_ssh_key_fingerprint()
 + gpg-interface: fix leak of "line" in parse_ssh_output()
  (merged to 'next' on 2021-10-14 at 97735c6091)
 + ssh signing: clarify trustlevel usage in docs
 + ssh signing: fmt-merge-msg tests & config parse
 + Merge branch 'fs/ssh-signing' into fs/ssh-signing-fix
 (this branch is used by fs/ssh-signing-key-lifetime; uses fs/ssh-signing.)
 Fix-up for the other topic already in 'next'.
* jc/doc-commit-header-continuation-line (2021-10-12) 1 commit
  (merged to 'next' on 2021-10-18 at 99b71c0aaf)
 + signature-format.txt: explain and illustrate multi-line headers
 Doc update.
* jh/perf-remove-test-times (2021-10-04) 1 commit
  (merged to 'next' on 2021-10-14 at 473a26166c)
 + t/perf/perf-lib.sh: remove test_times.* at the end test_perf_()
 Perf test fix.
* js/userdiff-cpp (2021-10-25) 7 commits
  (merged to 'next' on 2021-10-25 at 2158813163)
 + userdiff-cpp: back out the digit-separators in numbers
  (merged to 'next' on 2021-10-18 at fea77f6c4e)
 + userdiff-cpp: learn the C++ spaceship operator
 + userdiff-cpp: permit the digit-separating single-quote in numbers
 + userdiff-cpp: prepare test cases with yet unsupported features
 + userdiff-cpp: tighten word regex
 + t4034: add tests showing problematic cpp tokenizations
 + t4034/cpp: actually test that operator tokens are not split
 Userdiff patterns for the C++ language has been updated.
* jt/no-abuse-alternate-odb-for-submodules (2021-10-08) 9 commits
  (merged to 'next' on 2021-10-14 at ae49a2ceda)
 + submodule: trace adding submodule ODB as alternate
 + submodule: pass repo to check_has_commit()
 + object-file: only register submodule ODB if needed
 + merge-{ort,recursive}: remove add_submodule_odb()
 + refs: peeling non-the_repository iterators is BUG
 + refs: teach arbitrary repo support to iterators
 + refs: plumb repo into ref stores
 + Merge branch 'jk/ref-paranoia' into jt/no-abuse-alternate-odb-for-submodules
 + Merge branch 'jt/add-submodule-odb-clean-up' into jt/no-abuse-alternate-odb-for-submodules
 Follow through the work to use the repo interface to access
 submodule objects in-process, instead of abusing the alternate
 object database interface.
* pw/sparse-cache-tree-verify-fix (2021-10-18) 2 commits
  (merged to 'next' on 2021-10-18 at 0186a643cc)
 + t1092: run "rebase --apply" without "-q" in testing
  (merged to 'next' on 2021-10-11 at 2f90c87850)
 + sparse index: fix use-after-free bug in cache_tree_verify()
 Recent sparse-index addition, namely any use of index_name_pos(),
 can expand sparse index entries and breaks any code that walks
 cache-tree or existing index entries.  One such instance of such a
 breakage has been corrected.
* rs/add-dry-run-without-objects (2021-10-12) 1 commit
  (merged to 'next' on 2021-10-14 at a42928e134)
 + add: don't write objects with --dry-run
 Stop "git add --dry-run" from creating new blob and tree objects.
* rs/disable-gc-during-perf-tests (2021-10-11) 1 commit
  (merged to 'next' on 2021-10-14 at e0dd4b9bd4)
 + perf: disable automatic housekeeping
 Avoid performance measurements from getting ruined by gc and other
 housekeeping pauses interfering in the middle.
* tb/fix-midx-rename-while-mapped (2021-10-15) 5 commits
  (merged to 'next' on 2021-10-18 at 52e552caae)
 + midx.c: guard against commit_lock_file() failures
 + midx.c: lookup MIDX by object directory during repack
 + midx.c: lookup MIDX by object directory during expire
 + midx.c: extract MIDX lookup by object_dir
 + Merge branch 'tb/repack-write-midx' into tb/fix-midx-rename-while-mapped
 The codepath to write a new version of .midx multi-pack index files
 has learned to release the mmaped memory holding the current
 version of .midx before removing them from the disk, as some
 platforms do not allow removal of a file that still has mapping.

-------------------------------------------------- [New Topics]

* ab/generate-command-list (2021-10-23) 10 commits
 - generate-cmdlist.sh: replace "cut", "tr" and "grep" with pure-shell
 - generate-cmdlist.sh: replace "grep' invocation with a shell version
 - generate-cmdlist.sh: do not shell out to "sed"
 - generate-cmdlist.sh: stop sorting category lines
 - generate-cmdlist.sh: replace for loop by printf's auto-repeat feature
 - generate-cmdlist.sh: run "grep | sort", not "sort | grep"
 - generate-cmdlist.sh: don't call get_categories() from category_list()
 - generate-cmdlist.sh: spawn fewer processes
 - generate-cmdlist.sh: trivial whitespace change
 - command-list.txt: sort with "LC_ALL=C sort"
 Build optimization.
* ab/ref-filter-leakfix (2021-10-20) 3 commits
  (merged to 'next' on 2021-10-23 at 8066971a3d)
 + branch: use ref_sorting_release()
 + ref-filter API user: add and use a ref_sorting_release()
 + tag: use a "goto cleanup" pattern, leak less memory
 (this branch is used by jc/fix-ref-sorting-parse.)
 "git for-each-ref" family of commands were leaking the ref_sorting
 instances that hold sorting keys specified by the user; this has
 been corrected.
 Will merge to 'master'.
* bs/doc-blame-color-lines (2021-10-20) 1 commit
  (merged to 'next' on 2021-10-23 at 4da10a5162)
 + git config doc: fix recent ASCIIDOC formatting regression
 Doc fix.
 Will merge to 'master'.
* jc/fix-pull-ff-only-when-already-up-to-date (2021-10-20) 1 commit
 - pull: --ff-only should make it a noop when already-up-to-date
 "git pull --ff-only" and "git pull --rebase --ff-only" should make
 it a no-op to attempt pulling from a remote that is behind us, but
 instead the command errored out by saying it was impossible to
 fast-forward, which may technically be true, but not a useful thing
 to diagnose as an error.  This has been corrected.
 Will merge to 'next'?
* ab/make-sparse-for-real (2021-10-21) 1 commit
  (merged to 'next' on 2021-10-23 at f7a8389fb3)
 + Makefile: remove redundant GIT-CFLAGS dependency from "sparse"
 Fix-up for a recent topic.
 Will merge to 'master'.
* ab/plug-handle-path-exclude-leak (2021-10-21) 1 commit
  (merged to 'next' on 2021-10-23 at 6be5d7bea8)
 + config.c: don't leak memory in handle_path_include()
 Leakfix.
 Will merge to 'master'.
* ab/plug-random-leaks (2021-10-23) 6 commits
  (merged to 'next' on 2021-10-23 at 9c04a95718)
 + reflog: free() ref given to us by dwim_log()
 + submodule--helper: fix small memory leaks
 + clone: fix a memory leak of the "git_dir" variable
 + grep: fix a "path_list" memory leak
 + grep: use object_array_clear() in cmd_grep()
 + grep: prefer "struct grep_opt" over its "void *" equivalent
 Leakfix.
 Will merge to 'master'.
* ab/sh-retire-helper-functions (2021-10-21) 6 commits
 - git-sh-setup: remove "sane_grep", it's not needed anymore
 - git-sh-setup: remove unused sane_egrep() function
 - git-instaweb: unconditionally assume that gitweb is mod_perl capable
 - Makefile: remove $(NO_CURL) from $(SCRIPT_DEFINES)
 - Makefile: remove $(GIT_VERSION) from $(SCRIPT_DEFINES)
 - Makefile: move git-SCRIPT-DEFINES adjacent to $(SCRIPT_DEFINES)
 Make a few helper functions unused and then lose them.
* ab/sh-retire-rebase-preserve-merges (2021-10-21) 2 commits
  (merged to 'next' on 2021-10-23 at f1fede7500)
 + git-sh-setup: remove messaging supporting --preserve-merges
 + git-sh-i18n: remove unused eval_ngettext()
 Code clean-up to remove unused helpers.
 Will merge to 'master'.
* ow/stash-count-in-status-porcelain-output (2021-10-21) 2 commits
 - status: print stash info with --porcelain=v2 --show-stash
 - status: count stash entries in separate function
 Allow "git status --porcelain=v2" to show the number of stash
 entries with --show-stash like the normal output does.
 Will merge to 'next'.
* tb/plug-pack-bitmap-leaks (2021-10-21) 9 commits
 - pack-bitmap.c: more aggressively free in free_bitmap_index()
 - pack-bitmap.c: don't leak type-level bitmaps
 - pack-bitmap.c: avoid leaking via midx_bitmap_filename()
 - builtin/multi-pack-index.c: don't leak concatenated options
 - builtin/repack.c: avoid leaking child arguments
 - builtin/pack-objects.c: don't leak memory via arguments
 - t/helper/test-read-midx.c: free MIDX within read_midx_file()
 - midx.c: don't leak MIDX from verify_midx_file
 - midx.c: clean up chunkfile after reading the MIDX
 Leakfix.
 Will merge to 'next'?
* bs/archive-doc-compression-level (2021-10-25) 1 commit
  (merged to 'next' on 2021-10-25 at 9360d864a2)
 + archive: describe compression level option
 Update "git archive" documentation and give explicit mention on the
 compression level for both zip and tar.gz format.
 Will merge to 'master'.
* es/pretty-describe-more (2021-10-23) 3 commits
 - pretty: add abbrev option to %(describe)
 - pretty: add tag option to %(describe)
 - pretty.c: rename describe options variable to more descriptive name
 Extend "git log --format=%(describe)" placeholder to allow passing
 selected command-line options to the underlying "git describe"
 command.
 Expecting a reroll.
* fs/ssh-signing-key-lifetime (2021-10-23) 8 commits
 - ssh signing: fmt-merge-msg/check_signature with tag date
 - ssh signing: verify-tag/check_signature with tag date
 - ssh signing: git log/check_signature with commit date
 - ssh signing: verify-commit/check_signature with commit date
 - ssh signing: add key lifetime test prereqs
 - ssh signing: extend check_signature to accept payload metadata
 - Merge branch 'fs/ssh-signing-fix' into fs/ssh-signing-key-lifetime
 - Merge branch 'fs/ssh-signing' into fs/ssh-signing-key-lifetime
 Extend the signing of objects with SSH keys and learn to pay
 attention to the key validity time range when verifying.
 Expecting a reroll.
 cf. <51099904-a962-eb23-8baf-9ce15fff7d10@gigacodes.de>
* jc/branch-copy-doc (2021-10-23) 1 commit
 - branch (doc): -m/-c copies config and reflog
 "git branch -c/-m new old" was not described to copy config, which
 has been corrected.
 Will merge to 'next'?
* jc/doc-format-patch-clarify-auto-base (2021-10-23) 1 commit
 - format-patch (doc): clarify --base=auto
 Rephrase the description of "format-patch --base=auto".
 Will merge to 'next'?
* jc/doc-submitting-patches-choice-of-base (2021-10-25) 2 commits
 - (wip) reword the final review part
 - SubmittingPatchs: clarify choice of base and testing
 Extend the guidance to choose the base commit to build your work
 on, and hint/nudge contributors to read others' changes.
* js/expand-runtime-prefix (2021-10-25) 1 commit
  (merged to 'next' on 2021-10-25 at 7ff05e8222)
 + config.txt: fix typo
 Typofix.
 Will merge to 'master'.
* ma/doc-folder-to-directory (2021-10-25) 3 commits
 - gitweb.txt: change "folder" to "directory"
 - gitignore.txt: change "folder" to "directory"
 - git-multi-pack-index.txt: change "folder" to "directory"
 Consistently use 'directory', not 'folder', to call the filesystem
 entity that collects a group of files and, eh, directories.
 Will merge to 'next' and then to 'master'.
* ma/doc-git-version (2021-10-25) 1 commit
  (merged to 'next' on 2021-10-25 at 9f74afec0c)
 + git.txt: fix typo
 Typofix.
 Will merge to 'master'.
* rd/http-backend-code-simplification (2021-10-25) 1 commit
 - http-backend: remove a duplicated code branch
 (slight) Code simplification.
 Will merge to 'next'?
* sg/sparse-index-not-that-common-a-command (2021-10-25) 1 commit
 - command-list.txt: remove 'sparse-index' from main help
 Drop "git sparse-index" from the list of common commands.
 Will merge to 'next'.

-------------------------------------------------- [Stalled]

* ar/submodule-update (2021-10-13) 9 commits
 . submodule--helper: rename helper functions
 . submodule--helper: remove unused helpers
 . submodule: move core cmd_update() logic to C
 . submodule--helper: run update using child process struct
 . submodule--helper: allow setting superprefix for init_submodule()
 . submodule--helper: refactor get_submodule_displaypath()
 . submodule--helper: rename helpers for update-clone
 . submodule--helper: get remote names from any repository
 . submodule--helper: split up ensure_core_worktree()
 Rewrite of "git submodule update" in C.
 Kicked out of 'seen' to make room for es/superproject-aware-submodules
 which is among the topics this topic stomps on.
* pw/diff-color-moved-fix (2021-08-05) 13 commits
 - diff: drop unused options parameter from cmp_in_block_with_wsd()
 - diff --color-moved: intern strings
 - diff: use designated initializers for emitted_diff_symbol
 - diff --color-moved-ws=allow-indentation-change: improve hash lookups
 - diff --color-moved: stop clearing potential moved blocks
 - diff --color-moved: shrink potential moved blocks as we go
 - diff --color-moved: unify moved block growth functions
 - diff --color-moved: call comparison function directly
 - diff --color-moved-ws=allow-indentation-change: simplify and optimize
 - diff: simplify allow-indentation-change delta calculation
 - diff --color-moved: avoid false short line matches and bad zerba coloring
 - diff --color-moved=zebra: fix alternate coloring
 - diff --color-moved: add perf tests
 Originally merged to 'next' on 2021-08-05
 Long-overdue correctness and performance update to "diff
 --color-moved" feature.
 Expecting a reroll.
 cf. <8bec1a6d-5052-50c3-4100-e6348289d581@gmail.com>
* cf/fetch-set-upstream-while-detached (2021-07-06) 1 commit
 - fetch: fix segfault on --set-upstream while on a detached HEAD
 "git fetch --set-upstream" while on detached HEAD segfaulted
 instead of noticing that such an operation did not make sense.
 Expecting a reroll.
 cf. <xmqqsg0ri5mq.fsf@gitster.g>

-------------------------------------------------- [Cooking]

* gc/remote-with-fewer-static-global-variables (2021-10-20) 4 commits
 - remote: add struct repository parameter to external functions
 - remote: remove the_repository->remote_state from static methods
 - remote: use remote_state parameter internally
 - remote: move static variables into per-repository struct
 Code clean-up to eventually allow information on remotes defined
 for an arbitrary repository to be read.
 Expecting a reroll.
 Breaks pushremote_get() and causes "git push" to segfault; there
 might be other similar breakages.
 cf. <xmqqtuhbo2tn.fsf@gitster.g>
* jk/loosen-urlmatch (2021-10-12) 1 commit
  (merged to 'next' on 2021-10-25 at f66ca39ebe)
 + urlmatch: add underscore to URL_HOST_CHARS
 Treat "_" as any other URL-valid characters in an URL when matching
 the per-URL configuration variable names.
 Will merge to 'master'.
* ab/test-bail (2021-10-14) 2 commits
  (merged to 'next' on 2021-10-23 at 4a16ebdb74)
 + test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use
 + test-lib.sh: de-duplicate error() teardown code
 A new feature has been added to abort early in the test framework.
 Will merge to 'master'.
* ab/config-based-hooks-2 (2021-10-20) 14 commits
 - run-command: remove old run_hook_{le,ve}() hook API
 - receive-pack: convert push-to-checkout hook to hook.h
 - read-cache: convert post-index-change to use hook.h
 - commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
 - git-p4: use 'git hook' to run hooks
 - send-email: use 'git hook run' for 'sendemail-validate'
 - git hook run: add an --ignore-missing flag
 - merge: convert post-merge to use hook.h
 - hooks: convert 'post-checkout' hook to hook library
 - am: convert applypatch to use hook.h
 - rebase: convert pre-rebase to use hook.h
 - gc: use hook library for pre-auto-gc hook
 - hook: add 'run' subcommand
 - Merge branch 'ab/config-based-hooks-1' into ab/config-based-hooks-2
 More "config-based hooks".
* ab/ignore-replace-while-working-on-commit-graph (2021-10-15) 3 commits
  (merged to 'next' on 2021-10-25 at 5ed4266a96)
 + commit-graph: don't consider "replace" objects with "verify"
 + commit-graph tests: fix another graph_git_two_modes() helper
 + commit-graph tests: fix error-hiding graph_git_two_modes() helper
 (this branch is used by gc/use-repo-settings.)
 Teach "git commit-graph" command not to allow using replace objects
 at all, as we do not use the commit-graph at runtime when we see
 object replacement.
 Will merge to 'master'.
* so/stash-staged (2021-10-18) 1 commit
  (merged to 'next' on 2021-10-25 at 68b88e35c4)
 + stash: implement '--staged' option for 'push' and 'save'
 "git stash" learned the "--staged" option to stash away what has
 been added to the index (and nothing else).
 Will merge to 'master'.
* vd/sparse-sparsity-fix-on-read (2021-10-21) 3 commits
 - sparse-index: update do_read_index to ensure correct sparsity
 - sparse-index: add ensure_correct_sparsity function
 - test-read-cache.c: prepare_repo_settings after config init
 Ensure that the sparseness of the in-core index matches the
 index.sparse configuration specified by the repository immediately
 after the on-disk index file is read.
* jc/fix-ref-sorting-parse (2021-10-20) 2 commits
 - for-each-ref: delay parsing of --sort=<atom> options
 - Merge branch 'ab/ref-filter-leakfix' into jc/fix-ref-sorting-parse
 (this branch uses ab/ref-filter-leakfix.)
 Things like "git -c branch.sort=bogus branch new HEAD", i.e. the
 operation modes of the "git branch" command that do not need the
 sort key information, no longer errors out by seeing a bogus sort
 key.
 Will merge to 'next'.
* jc/tutorial-format-patch-base (2021-10-23) 1 commit
 - MyFirstContribution: teach to use "format-patch --base=auto"
 Teach and encourage first-time contributors to this project to
 state the base commit when they submit their topic.
 Will merge to 'next'.
* jk/http-push-status-fix (2021-10-18) 2 commits
  (merged to 'next' on 2021-10-23 at 9704ff261d)
 + transport-helper: recognize "expecting report" error from send-pack
 + send-pack: complain about "expecting report" with --helper-status
 "git push" client talking to an HTTP server did not diagnose the
 lack of the final status report from the other side correctly,
 which has been corrected.
 Will merge to 'master'.
* js/branch-track-inherit (2021-10-18) 1 commit
 - branch: add flags and config to inherit tracking
 "git -c branch.autosetupmerge=inherit branch new old" makes "new"
 to have the same upstream as the "old" branch, instead of marking
 "old" itself as its upstream.
 Under discussion.
 cf. <87a6j6tbsv.fsf@gmgdl.gmail.com>
* rb/doc-commit-header-continuation-line (2021-10-11) 1 commit
 - signature-format.txt: add space to fix gpgsig continuation line
 Values in the header portion of commit object can be multi-lined
 by a single SP indentation of the second and subsequent lines, and
 this applies to an empty line as well.  Update an example in the
 technical documentation to highlight it.
 Will discard.
 Superseded by the jc/doc-commit-header-continuation-line topic.
* jh/builtin-fsmonitor-part2 (2021-10-21) 29 commits
 - t7527: test status with untracked-cache and fsmonitor--daemon
 - fsmonitor: force update index after large responses
 - fsmonitor--daemon: use a cookie file to sync with file system
 - fsmonitor--daemon: periodically truncate list of modified files
 - t/perf/p7519: add fsmonitor--daemon test cases
 - t/perf/p7519: speed up test on Windows
 - t/helper/test-chmtime: skip directories on Windows
 - t/perf: avoid copying builtin fsmonitor files into test repo
 - t7527: create test for fsmonitor--daemon
 - t/helper/fsmonitor-client: create IPC client to talk to FSMonitor Daemon
 - help: include fsmonitor--daemon feature flag in version info
 - fsmonitor--daemon: implement handle_client callback
 - compat/fsmonitor/fsm-listen-darwin: implement FSEvent listener on MacOS
 - compat/fsmonitor/fsm-listen-darwin: add macos header files for FSEvent
 - compat/fsmonitor/fsm-listen-win32: implement FSMonitor backend on Windows
 - fsmonitor--daemon: create token-based changed path cache
 - fsmonitor--daemon: define token-ids
 - fsmonitor--daemon: add pathname classification
 - fsmonitor--daemon: implement 'start' command
 - fsmonitor--daemon: implement 'run' command
 - compat/fsmonitor/fsm-listen-darwin: stub in backend for Darwin
 - compat/fsmonitor/fsm-listen-win32: stub in backend for Windows
 - fsmonitor--daemon: implement 'stop' and 'status' commands
 - fsmonitor--daemon: add a built-in fsmonitor daemon
 - fsmonitor: document builtin fsmonitor
 - fsmonitor: use IPC to query the builtin FSMonitor daemon
 - fsmonitor: config settings are repository-specific
 - fsmonitor-ipc: create client routines for git-fsmonitor--daemon
 - fsmonitor: enhance existing comments
 Built-in fsmonitor (part 2).
* ld/sparse-diff-blame (2021-10-15) 3 commits
 - blame: enable and test the sparse index
 - diff: enable and test the sparse index
 - Merge branch 'vd/sparse-reset' into ld/sparse-diff-blame
 (this branch uses vd/sparse-reset.)
 Teach diff and blame to work well with sparse index.
* mp/absorb-submodule-git-dir-upon-deinit (2021-10-07) 1 commit
 - submodule: absorb git dir instead of dying on deinit
 "git submodule deinit" for a submodule whose .git metadata
 directory is embedded in its working tree refused to work, until
 the submodule gets converted to use the "absorbed" form where the
 metadata directory is stored in superproject, and a gitfile at the
 top-level of the working tree of the submodule points at it.  The
 command is taught to convert such submodules to the absorbed form
 as needed.
 Under review.
 cf. <xmqqwnmopqqk.fsf@gitster.g>
* ns/remerge-diff (2021-10-08) 8 commits
 - doc/diff-options: explain the new --remerge-diff option
 - show, log: adapt Elijah Newren's changes to common tmp-objdir API
 - show, log: provide a --remerge-diff capability
 - merge-ort: capture and print ll-merge warnings in our preferred fashion
 - ll-merge: add API for capturing warnings in a strbuf instead of stderr
 - merge-ort: add ability to record conflict messages in a file
 - merge-ort: mark a few more conflict messages as omittable
 - Merge branch 'ns/tmp-objdir' into ns/remerge-diff
 (this branch uses ns/tmp-objdir.)
 A new presentation for two-parent merge "--remerge-diff" can be
 used to show the difference between mechanical (and possibly
 conflicted) merge results and the recorded resolution.
 On hold.
 This is Elijah's remerge-diff rebased on ns/tmp-objdir to share the
 "create objects temporarily, only to discard without committing
 them to longer-term storage" infrastructure with another topic.
* ns/tmp-objdir (2021-10-04) 2 commits
  (merged to 'next' on 2021-10-23 at 358d376f61)
 + tmp-objdir: disable ref updates when replacing the primary odb
 + tmp-objdir: new API for creating temporary writable databases
 (this branch is used by ns/batched-fsync and ns/remerge-diff.)
 New interface into the tmp-objdir API to help in-core use of the
 quarantine feature.
 Will merge to 'master'.
* vd/sparse-reset (2021-10-11) 8 commits
 - unpack-trees: improve performance of next_cache_entry
 - reset: make --mixed sparse-aware
 - reset: make sparse-aware (except --mixed)
 - reset: integrate with sparse index
 - reset: expand test coverage for sparse checkouts
 - sparse-index: update command for expand/collapse test
 - reset: preserve skip-worktree bit in mixed reset
 - reset: rename is_missing to !is_in_reset_tree
 (this branch is used by ld/sparse-diff-blame.)
 Various operating modes of "git reset" have been made to work
 better with the sparse index.
 Needs review.
* gc/use-repo-settings (2021-10-15) 4 commits
  (merged to 'next' on 2021-10-25 at 3d38c09a8d)
 + gc: perform incremental repack when implictly enabled
 + fsck: verify multi-pack-index when implictly enabled
 + fsck: verify commit graph when implicitly enabled
 + Merge branch 'ab/ignore-replace-while-working-on-commit-graph' into gc/use-repo-settings
 (this branch uses ab/ignore-replace-while-working-on-commit-graph.)
 It is wrong to read some settings directly from the config
 subsystem, as things like feature.experimental can affect their
 default values.
 Will merge to 'master'.
* pw/fix-some-issues-in-reset-head (2021-10-01) 12 commits
 - rebase -m: don't fork git checkout
 - rebase --apply: set ORIG_HEAD correctly
 - rebase --apply: fix reflog
 - reset_head(): take struct rebase_head_opts
 - rebase: cleanup reset_head() calls
 - reset_head(): make default_reflog_action optional
 - reset_head(): factor out ref updates
 - reset_head(): remove action parameter
 - reset_head(): don't run checkout hook if there is an error
 - reset_head(): fix checkout
 - rebase: factor out checkout for up to date branch
 - Merge branch 'pw/rebase-of-a-tag-fix' into pw/fix-some-issues-in-reset-head
 Fix "some issues" in a helper function reset_head().
 Expecting a reroll.
 Needs a lot better explanation, including what the issues are,
 which codepaths the helper is used and to do what, and tests to
 protect the fixes.
* es/superproject-aware-submodules (2021-10-14) 4 commits
 - submodule: record superproject gitdir during 'update'
 - submodule: record superproject gitdir during absorbgitdirs
 - introduce submodule.superprojectGitDir record
 - t7400-submodule-basic: modernize inspect() helper
 A configuration variable in a submodule points at the location of
 the superproject it is bound to (RFC).
 Under discussion.
 cf. <911ab2c1-8a11-d9d0-4b28-fc801112f6da@gmail.com>
* tp/send-email-completion (2021-10-07) 3 commits
 - send-email docs: add format-patch options
 - send-email: programmatically generate bash completions
 - send-email: terminate --git-completion-helper with LF
 The command line complation for "git send-email" options have been
 tweaked to make it easier to keep it in sync with the command itself.
 Expecting a reroll.
 cf. <87fst7lkjx.fsf@evledraar.gmail.com>
* hm/paint-hits-in-log-grep (2021-10-15) 4 commits
  (merged to 'next' on 2021-10-25 at e3edea3fa9)
 + grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data
 + pretty: colorize pattern matches in commit messages
 + grep: refactor next_match() and match_one_pattern() for external use
 + Merge branch 'jk/grep-haystack-is-read-only' into hm/paint-hits-in-log-grep
 "git log --grep=string --author=name" learns to highlight hits just
 like "git grep string" does.
 Will merge to 'master'.
* ks/submodule-add-message-fix (2021-10-23) 1 commit
  (merged to 'next' on 2021-10-25 at 377e759528)
 + submodule--helper: fix incorrect newlines in an error message
 Message regression fix.
 Will merge to 'master'.
* ns/batched-fsync (2021-10-08) 8 commits
  (merged to 'next' on 2021-10-25 at e45c907d41)
 + core.fsyncobjectfiles: performance tests for add and stash
 + core.fsyncobjectfiles: tests for batch mode
 + unpack-objects: use the bulk-checkin infrastructure
 + update-index: use the bulk-checkin infrastructure
 + core.fsyncobjectfiles: add windows support for batch mode
 + core.fsyncobjectfiles: batched disk flushes
 + bulk-checkin: rename 'state' variable and separate 'plugged' boolean
 + Merge branch 'ns/tmp-objdir' into ns/batched-fsync
 (this branch uses ns/tmp-objdir.)
 The "core.fsyncobjectfiles" configuration variable can now be set
 to "batch" for improved performance.
 Will merge to 'master'.
* en/zdiff3 (2021-09-20) 2 commits
 - update documentation for new zdiff3 conflictStyle
 - xdiff: implement a zealous diff3, or "zdiff3"
 "Zealous diff3" style of merge conflict presentation has been added.
* js/scalar (2021-10-07) 15 commits
 - scalar: accept -C and -c options before the subcommand
 - scalar: implement the `version` command
 - scalar: implement the `delete` command
 - scalar: teach 'reconfigure' to optionally handle all registered enlistments
 - scalar: allow reconfiguring an existing enlistment
 - scalar: implement the `run` command
 - scalar: teach 'clone' to support the --single-branch option
 - scalar: implement the `clone` subcommand
 - scalar: implement 'scalar list'
 - scalar: let 'unregister' handle a deleted enlistment directory gracefully
 - scalar: 'unregister' stops background maintenance
 - scalar: 'register' sets recommended config and starts maintenance
 - scalar: create test infrastructure
 - scalar: start documenting the command
 - scalar: create a rudimentary executable
 Add pieces from "scalar" to contrib/.
 What's the status of this thing?
* ms/customizable-ident-expansion (2021-09-01) 1 commit
 - keyword expansion: make "$Id$" string configurable
 Instead of "$Id$", user-specified string (like $FreeBSD$) can be
 used as an in-blob placeholder for keyword expansion.
 What's the status of this one?  Meh?
* ab/refs-errno-cleanup (2021-10-16) 21 commits
 - refs API: post-migration API renaming [2/2]
 - refs API: post-migration API renaming [1/2]
 - refs API: don't expose "errno" in run_transaction_hook()
 - refs API: make expand_ref() & repo_dwim_log() not set errno
 - refs API: make resolve_ref_unsafe() not set errno
 - refs API: make refs_ref_exists() not set errno
 - refs API: make refs_resolve_refdup() not set errno
 - refs tests: ignore ignore errno in test-ref-store helper
 - refs API: ignore errno in worktree.c's find_shared_symref()
 - refs API: ignore errno in worktree.c's add_head_info()
 - refs API: make files_copy_or_rename_ref() et al not set errno
 - refs API: make loose_fill_ref_dir() not set errno
 - refs API: make resolve_gitlink_ref() not set errno
 - refs API: remove refs_read_ref_full() wrapper
 - refs/files: remove "name exist?" check in lock_ref_oid_basic()
 - reflog tests: add --updateref tests
 - refs API: make refs_rename_ref_available() static
 - refs API: make parse_loose_ref_contents() not set errno
 - refs API: make refs_read_raw_ref() not set errno
 - refs API: add a version of refs_resolve_ref_unsafe() with "errno"
 - branch tests: test for errno propagating on failing read
 The "remainder" of hn/refs-errno-cleanup topic.
 Will merge to 'next'.
* ab/only-single-progress-at-once (2021-10-13) 10 commits
 - progress.c: add & assert a "global_progress" variable
 - various *.c: use isatty(1|2), not isatty(STDIN_FILENO|STDERR_FILENO)
 - pack-bitmap-write.c: don't return without stop_progress()
 - progress.c: add temporary variable from progress struct
 - progress.c: call progress_interval() from progress_test_force_update()
 - progress.c: move signal handler functions lower
 - progress.c tests: test some invalid usage
 - progress.c tests: make start/stop verbs on stdin
 - progress.c test helper: add missing braces
 - leak tests: fix a memory leaks in "test-progress" helper
 Further tweaks on progress API.
 Need to pick up an updated version.
 cf. <cover-v4-0.8-00000000000-20211025T111915Z-avarab@gmail.com>
* hn/reftable (2021-10-08) 19 commits
 - Add "test-tool dump-reftable" command.
 - reftable: add dump utility
 - reftable: implement stack, a mutable database of reftable files.
 - reftable: implement refname validation
 - reftable: add merged table view
 - reftable: add a heap-based priority queue for reftable records
 - reftable: reftable file level tests
 - reftable: read reftable files
 - reftable: generic interface to tables
 - reftable: write reftable files
 - reftable: a generic binary tree implementation
 - reftable: reading/writing blocks
 - Provide zlib's uncompress2 from compat/zlib-compat.c
 - reftable: (de)serialization for the polymorphic record type.
 - reftable: add blocksource, an abstraction for random access reads
 - reftable: utility functions
 - reftable: add error related functionality
 - reftable: add LICENSE
 - hash.h: provide constants for the hash IDs
 The "reftable" backend for the refs API, without integrating into
 the refs subsystem.

-------------------------------------------------- [Discarded]

* en/remerge-diff (2021-08-31) 7 commits
 . doc/diff-options: explain the new --remerge-diff option
 . show, log: provide a --remerge-diff capability
 . tmp-objdir: new API for creating and removing primary object dirs
 . merge-ort: capture and print ll-merge warnings in our preferred fashion
 . ll-merge: add API for capturing warnings in a strbuf instead of stderr
 . merge-ort: add ability to record conflict messages in a file
 . merge-ort: mark a few more conflict messages as omittable
 A new presentation for two-parent merge "--remerge-diff" can be
 used to show the difference between mechanical (and possibly
 conflicted) merge results and the recorded resolution.
Jeff King· Oct 26, 2021, 05:25 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On Mon, Oct 25, 2021 at 08:48:04PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> * ks/submodule-add-message-fix (2021-10-23) 1 commit
>   (merged to 'next' on 2021-10-25 at 377e759528)
>  + submodule--helper: fix incorrect newlines in an error message
> 
>  Message regression fix.
> 
>  Will merge to 'master'.

This commit has an extra unused parameter in the helper function. I think we'd want the patch below on top.

-- >8 --
Subject: submodule: drop unused sm_name parameter from append_fetch_remotes()

Commit c21fb4676f (submodule--helper: fix incorrect newlines in an error message, 2021-10-23) accidentally added a new, unused parameter while changing the name and signature of show_fetch_remotes() to append_fetch_remotes(). We can drop this to keep things simpler (and satisfy -Wunused-parameter).

The error is likely because c21fb4676f is fixing a problem from 8c8195e9c3 (submodule--helper: introduce add-clone subcommand, 2021-07-10). An earlier iteration of that second commit introduced the same unused parameter (though it was dropped before it finally made it to 'next'), and the fix on top accidentally carried forward the extra parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
If this seems familiar, it's because the fixup for the earlier commit
was discussed in July:
  https://lore.kernel.org/git/YPqkHs47VDFBNZ0Z@coredump.intra.peff.net/
 builtin/submodule--helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5224283bd1..13a098305b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2776,7 +2776,7 @@ struct add_data {
 };
 #define ADD_DATA_INIT { .depth = -1 }
 
-static void append_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)
+static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
 {
 	struct child_process cp_remote = CHILD_PROCESS_INIT;
 	struct strbuf sb_remote_out = STRBUF_INIT;
@@ -2831,8 +2831,7 @@ static int add_submodule(const struct add_data *add_data)
 						    "locally with remote(s):\n"),
 					    add_data->sm_name);
 
-				append_fetch_remotes(&msg, add_data->sm_name,
-						     submod_gitdir_path);
+				append_fetch_remotes(&msg, submod_gitdir_path);
 				free(submod_gitdir_path);
 
 				strbuf_addf(&msg, _("If you want to reuse this local git "
-- 
2.33.1.1326.g74374c95d7
Junio C Hamano· Oct 27, 2021, 17:42 UTC · re: Jeff King · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> On Mon, Oct 25, 2021 at 08:48:04PM -0700, Junio C Hamano wrote:
>
>> * ks/submodule-add-message-fix (2021-10-23) 1 commit
>>   (merged to 'next' on 2021-10-25 at 377e759528)
>>  + submodule--helper: fix incorrect newlines in an error message
>> 
>>  Message regression fix.
>> 
>>  Will merge to 'master'.
>
> This commit has an extra unused parameter in the helper function. I
> think we'd want the patch below on top.
Thanks.
Kaartic Sivaraam· Oct 31, 2021, 18:36 UTC · re: Jeff King · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On 26/10/21 10:55 am, Jeff King wrote:
Show 13 quoted lines
> On Mon, Oct 25, 2021 at 08:48:04PM -0700, Junio C Hamano wrote:
> 
>> * ks/submodule-add-message-fix (2021-10-23) 1 commit
>>    (merged to 'next' on 2021-10-25 at 377e759528)
>>   + submodule--helper: fix incorrect newlines in an error message
>>
>>   Message regression fix.
>>
>>   Will merge to 'master'.
> 
> This commit has an extra unused parameter in the helper function. I
> think we'd want the patch below on top.
> 
Sorry for the oversight and thanks for catching it (again)!

This reminded me to check my config.mak and enable DEVELOPER=1 which I seem to have turned off for some reason. I did notice we explicitly add -Wno-unused-parameter to DEVELOPER_CFLAGS (likely with good reason). The rest of the flags should be helpful though :)

-- 
Sivaraam
Jeff King· Nov 1, 2021, 04:04 UTC · re: Kaartic Sivaraam · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On Mon, Nov 01, 2021 at 12:06:20AM +0530, Kaartic Sivaraam wrote:
Show 10 quoted lines
> > This commit has an extra unused parameter in the helper function. I
> > think we'd want the patch below on top.
> > 
> 
> Sorry for the oversight and thanks for catching it (again)!
> 
> This reminded me to check my config.mak and enable DEVELOPER=1
> which I seem to have turned off for some reason. I did notice
> we explicitly add -Wno-unused-parameter to DEVELOPER_CFLAGS (likely
> with good reason). The rest of the flags should be helpful though :)

Yes, there are hundreds of false positives for -Wunused-parameter. I have a series annotating all of them (which is indeed how I found the problem fixed here), but I haven't quite gotten around to cleaning them all up. (Plus I've been a little afraid to flood the list with them).

-Peff
Ævar Arnfjörð Bjarmason· Oct 26, 2021, 11:02 UTC · re: Junio C Hamano · lore

pre-v2.34.0-rc0 regressions: t7900-maintenance.sh broken due to 'systemd-analyze' (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Mon, Oct 25 2021, Junio C Hamano wrote:
[Spinning off some "before rc0" threads]
> The fifteenth batch of topics are in 'master'.  I expect that this
> is more-or-less what we can expect in the -rc0, unless there is a
> hotfix to what's already merged.

I reported a breakage with $subject in [1], which I see Lénaïc tried to fix in [2], but which I managed to miss. Sorry about that.

On that machine (gcc135, on the GCC farm) it's still breaking, relevant
trace output:
    
    ++ printf '%s\n' 'prerequisite SYSTEMD_ANALYZE ok'
    prerequisite SYSTEMD_ANALYZE ok
    ++ return 0
    ++ test_set_prereq SYSTEMD_ANALYZE
    ++ test -n ''
    ++ case "$1" in
    ++ satisfied_prereq=' POSIXPERM BSLASHPSPEC EXECKEEPSPID REFFILES COLUMNS_CAN_BE_1 PERL PTHREADS PYTHON GETTEXT XMLLINT SYSTEMD_ANALYZE '
    ++ lazily_tested_prereq='EXPENSIVE XMLLINT SYSTEMD_ANALYZE '
    ++ total_prereq=1
    ++ case "$satisfied_prereq" in
    ++ satisfied_this_prereq=t
    ++ case "$satisfied_this_prereq,$negative_prereq" in
    ++ ok_prereq=1
    ++ test 1 = 1
    ++ systemd-analyze verify systemd/user/git-maintenance@.service
    Failed to open /dev/tty0: Permission denied
    Failed to load systemd/user/git-maintenance@.service: Invalid argument
    error: last command exited with $?=1
    not ok 34 - start and stop Linux/systemd maintenance

I haven't looked into it, presumably easy-ish to fix, just moving on now and checking for other regressions...

1. https://lore.kernel.org/git/874ka618n4.fsf@evledraar.gmail.com/#t
2. https://lore.kernel.org/git/20210927213016.21714-2-lenaic@lhuard.fr/
Ævar Arnfjörð Bjarmason· Oct 26, 2021, 15:34 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: pre-v2.34.0-rc0 regressions: t7900-maintenance.sh broken due to 'systemd-analyze' (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

A On Tue, Oct 26 2021, Ævar Arnfjörð Bjarmason wrote:

Show 39 quoted lines
> On Mon, Oct 25 2021, Junio C Hamano wrote:
>
> [Spinning off some "before rc0" threads]
>
>> The fifteenth batch of topics are in 'master'.  I expect that this
>> is more-or-less what we can expect in the -rc0, unless there is a
>> hotfix to what's already merged.
>
> I reported a breakage with $subject in [1], which I see Lénaïc tried to
> fix in [2], but which I managed to miss. Sorry about that.
>
> On that machine (gcc135, on the GCC farm) it's still breaking, relevant
> trace output:
>     
>     ++ printf '%s\n' 'prerequisite SYSTEMD_ANALYZE ok'
>     prerequisite SYSTEMD_ANALYZE ok
>     ++ return 0
>     ++ test_set_prereq SYSTEMD_ANALYZE
>     ++ test -n ''
>     ++ case "$1" in
>     ++ satisfied_prereq=' POSIXPERM BSLASHPSPEC EXECKEEPSPID REFFILES COLUMNS_CAN_BE_1 PERL PTHREADS PYTHON GETTEXT XMLLINT SYSTEMD_ANALYZE '
>     ++ lazily_tested_prereq='EXPENSIVE XMLLINT SYSTEMD_ANALYZE '
>     ++ total_prereq=1
>     ++ case "$satisfied_prereq" in
>     ++ satisfied_this_prereq=t
>     ++ case "$satisfied_this_prereq,$negative_prereq" in
>     ++ ok_prereq=1
>     ++ test 1 = 1
>     ++ systemd-analyze verify systemd/user/git-maintenance@.service
>     Failed to open /dev/tty0: Permission denied
>     Failed to load systemd/user/git-maintenance@.service: Invalid argument
>     error: last command exited with $?=1
>     not ok 34 - start and stop Linux/systemd maintenance
>
> I haven't looked into it, presumably easy-ish to fix, just moving on now
> and checking for other regressions...
>
> 1. https://lore.kernel.org/git/874ka618n4.fsf@evledraar.gmail.com/#t
> 2. https://lore.kernel.org/git/20210927213016.21714-2-lenaic@lhuard.fr/

I looked into this a little bit, the immediate problem is that the prereq testing is different from the command we actually run, and I can run one but not the other on that box for some reason. I.e.

    [avar@gcc135 t]$ systemd-analyze verify /lib/systemd/system/basic.target; echo $?
    Failed to open /dev/tty0: Permission denied
    0
    
    [avar@gcc135 t]$ systemd-analyze verify systemd/user/git-maintenance@.service; echo $?
    Failed to open /dev/tty0: Permission denied
    Failed to load systemd/user/git-maintenance@.service: Invalid argument
    1

So "fixing" that is easy, just have the prereq test that thing in particular, and why does one thing have a /lib/ prefix, but not the other?

But presumably this points to a bigger problem. I.e. we just did a "git maintenance start" a few lines earlier.

If I could start something via systemd then presumably it's either up, or our "start" is buggy and we didn't actually start something, or are using the wrong (non-portable?) invocation to check the running status of the thing we just started?

Also aside from that shouldn't this be:
    test_when_finished "systemd-something stop-it some-service" &&
    systemd-something start-it some-service &&
    [...]

Or are runaway services handled somehow by systemd magic (tied to the PID of the test run?).

So in trying to fix it myself I ran into the boundaries of the little systemd knowledge I've got.

Eric Sunshine· Oct 26, 2021, 18:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: pre-v2.34.0-rc0 regressions: t7900-maintenance.sh broken due to 'systemd-analyze' (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Tue, Oct 26, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines
> [...]
> So "fixing" that is easy, just have the prereq test that thing in
> particular, and why does one thing have a /lib/ prefix, but not the
> other?
>
> But presumably this points to a bigger problem. I.e. we just did a "git
> maintenance start" a few lines earlier.
>
> If I could start something via systemd then presumably it's either up,
> or our "start" is buggy and we didn't actually start something, or are
> using the wrong (non-portable?) invocation to check the running status
> of the thing we just started?

The bit you're missing is that the test script only mocks up starting and stopping the systemd unit:

    write_script print-args <<-\EOF &&
    printf "%s\n" "$*" >>args
    EOF
    GIT_TEST_MAINT_SCHEDULER="systemctl:./print-args" \
        git maintenance start --scheduler=systemd-timer &&

So, it's only running the `print-args` script, not actually invoking `systemctl`.

On the other hand, the "analyze" check really is invoking a real systemd command. (Why `systemd-analyze` needs to open /dev/tty0 explicitly rather than just emitting to stdout or stderr is a different question...)

Show 8 quoted lines
> Also aside from that shouldn't this be:
>
>     test_when_finished "systemd-something stop-it some-service" &&
>     systemd-something start-it some-service &&
>     [...]
>
> Or are runaway services handled somehow by systemd magic (tied to the
> PID of the test run?).

Answered above: we're just mocking starting/stopping the unit, not actually launching any background services.

Ævar Arnfjörð Bjarmason· Nov 2, 2021, 14:24 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH] maintenance tests: fix systemd v2.34.0-rc* test regression

Fix tests added in b681b191f92 (maintenance: add support for systemd timers on Linux, 2021-09-04) to run successfully no systems where systemd-analyze is installed, but on which there's a discrepancy between a FILE argument of "/lib/systemd/system/basic.target" and "systemd/user/git-maintenance@.service" succeeding.

There was an attempt to work around previous breakage in these tests in 670e5973992 (maintenance: fix test t7900-maintenance.sh, 2021-09-27), as noted in my [1] that commit is wrong about its assumption that we can use "/lib/systemd/system/basic.target" as a canary.argument.

To fix this let's adjust this test to test what it really should be testing: If we've got systemd-analyze reporting anything useful, we should use it to check the syntax of our just-generated "systemd/user/git-maintenance@.service" file.

Even on systems where this previously succeeded we weren't effectively doing that, because "systemd-analyze" will pass various syntax errors by and exit with a status code of 0, e.g. if the "[Unit]" section is replaced with a nonsensical "[HlaghUnfUnf]" section.

To do that ignore whatever exit code we get from "systemd-analyze verify", and filter its stderr output to extract the sorts of lines it emits no note syntax warnings and errors. We need to filter out "Failed to load", which would be emitted e.g. on the gcc135.fsffrance.org test box[1].

We also need to pipe this output to FD's 5 & 6, to avoid mixing up the trace output with our own output under "-x".

1. https://lore.kernel.org/git/211026.8635oo11jk.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Junio: I think you'll want this in v2.34.0 per the report in
<211026.8635oo11jk.gmgdl@evledraar.gmail.com>.
 t/t7900-maintenance.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 74aa6384755..5fe2ea03c1d 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -20,15 +20,16 @@ test_xmllint () {
 	fi
 }
 
-test_lazy_prereq SYSTEMD_ANALYZE '
-	systemd-analyze verify /lib/systemd/system/basic.target
-'
-
 test_systemd_analyze_verify () {
-	if test_have_prereq SYSTEMD_ANALYZE
-	then
-		systemd-analyze verify "$@"
-	fi
+	# Ignoring any errors from systemd-analyze is intentional
+	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
+
+	cat systemd.out >&5 &&
+	sed -n \
+		-e '/^Failed to load/d' \
+		-e '/git-maintenance@i*\.service:/x' \
+		<systemd.err >&6 &&
+	rm systemd.out systemd.err
 }
 
 test_expect_success 'help text' '
@@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
+	# If we have a systemd-analyze on the system we can verify the
+	# generated file.
+	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err &&
 
 	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
 	test_cmp expect args &&
-- 
2.33.1.1570.g069344fdd45
Eric Sunshine· Nov 3, 2021, 05:09 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] maintenance tests: fix systemd v2.34.0-rc* test regression

On Tue, Nov 2, 2021 at 10:25 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> Fix tests added in b681b191f92 (maintenance: add support for systemd
> timers on Linux, 2021-09-04) to run successfully no systems where
s/no/on/
Show 23 quoted lines
> systemd-analyze is installed, but on which there's a discrepancy
> between a FILE argument of "/lib/systemd/system/basic.target" and
> "systemd/user/git-maintenance@.service" succeeding.
>
> There was an attempt to work around previous breakage in these tests
> in 670e5973992 (maintenance: fix test t7900-maintenance.sh,
> 2021-09-27), as noted in my [1] that commit is wrong about its
> assumption that we can use "/lib/systemd/system/basic.target" as a
> canary.argument.
>
> To fix this let's adjust this test to test what it really should be
> testing: If we've got systemd-analyze reporting anything useful, we
> should use it to check the syntax of our just-generated
> "systemd/user/git-maintenance@.service" file.
>
> Even on systems where this previously succeeded we weren't effectively
> doing that, because "systemd-analyze" will pass various syntax errors
> by and exit with a status code of 0, e.g. if the "[Unit]" section is
> replaced with a nonsensical "[HlaghUnfUnf]" section.
>
> To do that ignore whatever exit code we get from "systemd-analyze
> verify", and filter its stderr output to extract the sorts of lines it
> emits no note syntax warnings and errors. We need to filter out
s/no/on/
Show 9 quoted lines
> "Failed to load", which would be emitted e.g. on the
> gcc135.fsffrance.org test box[1].
>
> We also need to pipe this output to FD's 5 & 6, to avoid mixing up the
> trace output with our own output under "-x".
>
> 1. https://lore.kernel.org/git/211026.8635oo11jk.gmgdl@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason· Nov 10, 2021, 03:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

Fix tests added in b681b191f92 (maintenance: add support for systemd timers on Linux, 2021-09-04) to run successfully on systems where systemd-analyze is installed, but on which there's a discrepancy between a FILE argument of "/lib/systemd/system/basic.target" and "systemd/user/git-maintenance@.service" succeeding.

There was an attempt to work around previous breakage in these tests in 670e5973992 (maintenance: fix test t7900-maintenance.sh, 2021-09-27), as noted in my [1] that commit is wrong about its assumption that we can use "/lib/systemd/system/basic.target" as a canary.argument.

To fix this let's adjust this test to test what it really should be testing: If we've got systemd-analyze reporting anything useful, we should use it to check the syntax of our just-generated "systemd/user/git-maintenance@.service" file.

Even on systems where this previously succeeded we weren't effectively doing that, because "systemd-analyze" will pass various syntax errors by and exit with a status code of 0, e.g. if the "[Unit]" section is replaced with a nonsensical "[HlaghUnfUnf]" section.

To do that ignore whatever exit code we get from "systemd-analyze verify", and filter its stderr output to extract the sorts of lines it emits on note syntax warnings and errors. We need to filter out "Failed to load", which would be emitted e.g. on the gcc135.fsffrance.org test box[1].

We also need to pipe this output to FD's 5 & 6, to avoid mixing up the trace output with our own output under "-x".

1. https://lore.kernel.org/git/211026.8635oo11jk.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This test is still broken in rc2, this patch fixes it. The only update is a commit message grammar fix pointed out by Eric Sunshine, thanks!

Range-diff against v1:
1:  90172a8ddcc ! 1:  44f0cafa16e maintenance tests: fix systemd v2.34.0-rc* test regression
    @@ Commit message
         maintenance tests: fix systemd v2.34.0-rc* test regression
     
         Fix tests added in b681b191f92 (maintenance: add support for systemd
    -    timers on Linux, 2021-09-04) to run successfully no systems where
    +    timers on Linux, 2021-09-04) to run successfully on systems where
         systemd-analyze is installed, but on which there's a discrepancy
         between a FILE argument of "/lib/systemd/system/basic.target" and
         "systemd/user/git-maintenance@.service" succeeding.
    @@ Commit message
     
         To do that ignore whatever exit code we get from "systemd-analyze
         verify", and filter its stderr output to extract the sorts of lines it
    -    emits no note syntax warnings and errors. We need to filter out
    +    emits on note syntax warnings and errors. We need to filter out
         "Failed to load", which would be emitted e.g. on the
         gcc135.fsffrance.org test box[1].
     
 t/t7900-maintenance.sh | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 74aa6384755..5fe2ea03c1d 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -20,15 +20,16 @@ test_xmllint () {
 	fi
 }
 
-test_lazy_prereq SYSTEMD_ANALYZE '
-	systemd-analyze verify /lib/systemd/system/basic.target
-'
-
 test_systemd_analyze_verify () {
-	if test_have_prereq SYSTEMD_ANALYZE
-	then
-		systemd-analyze verify "$@"
-	fi
+	# Ignoring any errors from systemd-analyze is intentional
+	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
+
+	cat systemd.out >&5 &&
+	sed -n \
+		-e '/^Failed to load/d' \
+		-e '/git-maintenance@i*\.service:/x' \
+		<systemd.err >&6 &&
+	rm systemd.out systemd.err
 }
 
 test_expect_success 'help text' '
@@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	# start registers the repo
 	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
 
-	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
+	# If we have a systemd-analyze on the system we can verify the
+	# generated file.
+	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err &&
+	test_must_be_empty out &&
+	test_must_be_empty err &&
 
 	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
 	test_cmp expect args &&
-- 
2.34.0.rc2.791.gdbfcf909579
Johannes Schindelin· Nov 10, 2021, 13:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

Hi Ævar,
On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
Show 5 quoted lines
> Fix tests added in b681b191f92 (maintenance: add support for systemd
> timers on Linux, 2021-09-04) to run successfully on systems where
> systemd-analyze is installed, but on which there's a discrepancy
> between a FILE argument of "/lib/systemd/system/basic.target" and
> "systemd/user/git-maintenance@.service" succeeding.

Could you try to rephrase `there's a discrepancy between a FILE argument of "/lib/systemd/system/basic.target" and "systemd/user/git-maintenance@.service" succeeding` more clearly?

Also, commit messages in git.git should not assume that every reader has a profound knowledge of `systemd`. The commit message needs to do a better job at explaining what is broken in the first place. The CI runs pass, after all, so it is unclear that there is anything broken that would need fixing to begin with.

Show 10 quoted lines
> There was an attempt to work around previous breakage in these tests
> in 670e5973992 (maintenance: fix test t7900-maintenance.sh,
> 2021-09-27), as noted in my [1] that commit is wrong about its
> assumption that we can use "/lib/systemd/system/basic.target" as a
> canary.argument.
>
> To fix this let's adjust this test to test what it really should be
> testing: If we've got systemd-analyze reporting anything useful, we
> should use it to check the syntax of our just-generated
> "systemd/user/git-maintenance@.service" file.

What is "anything useful" here? And how can we use the output of `systemd-analyze` to check the syntax of the generated `.service` file? This is all very unclear.

Show 10 quoted lines
> Even on systems where this previously succeeded we weren't effectively
> doing that, because "systemd-analyze" will pass various syntax errors
> by and exit with a status code of 0, e.g. if the "[Unit]" section is
> replaced with a nonsensical "[HlaghUnfUnf]" section.
>
> To do that ignore whatever exit code we get from "systemd-analyze
> verify", and filter its stderr output to extract the sorts of lines it
> emits on note syntax warnings and errors. We need to filter out
> "Failed to load", which would be emitted e.g. on the
> gcc135.fsffrance.org test box[1].

The domain name is not as interesting as the verbatim error message would be.

> We also need to pipe this output to FD's 5 & 6, to avoid mixing up the
> trace output with our own output under "-x".

We do not need to pipe to file descriptors 5 and 6. Other file descriptors would do, either. We need to redirect away from 1 and 2, is what this sentence should say.

And the hint about `-x` suggests that even that is not true that we need to redirect 1, either, just 2. And we would only need to redirect 2 with shells that do not understand `BASH_XTRACEFD`. It would be best not to mention `-x` at all.

Show 19 quoted lines
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 74aa6384755..5fe2ea03c1d 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -20,15 +20,16 @@ test_xmllint () {
>  	fi
>  }
>
> -test_lazy_prereq SYSTEMD_ANALYZE '
> -	systemd-analyze verify /lib/systemd/system/basic.target
> -'
> -
>  test_systemd_analyze_verify () {
> -	if test_have_prereq SYSTEMD_ANALYZE
> -	then
> -		systemd-analyze verify "$@"
> -	fi
> +	# Ignoring any errors from systemd-analyze is intentional
> +	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
The semicolon is superfluous.

It is a bit sad that we're now doing unnecessary work when `systemd-analyze` does not even exist.

> +
> +	cat systemd.out >&5 &&
> +	sed -n \
> +		-e '/^Failed to load/d' \

Lines starting with the prefix `Failed to load` are now deleted. Okay. Nothing in the commit explains why we can safely ignore those messages, though.

> +		-e '/git-maintenance@i*\.service:/x' \

Lines containing `git-maintenance@.service:` (or the same pattern with an arbitrary number of `i`s after the `@` character???) are exchanged with hold space.

That does not look right.

Since this is a `sed -n` call, we would need an explicit `p` command to print anything. Therefore, the current code is a pretty expensive equivalent to calling `true >&6`: it succeeds, and it does not print anything.

Show 15 quoted lines
> +		<systemd.err >&6 &&
> +	rm systemd.out systemd.err
>  }
>
>  test_expect_success 'help text' '
> @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
>  	# start registers the repo
>  	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>
> -	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
> +	# If we have a systemd-analyze on the system we can verify the
> +	# generated file.
> +	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err &&

Since the function name has the suffix `_verify`, the verification part should be _inside_ that function, not outside.

This patch is not clear enough to tell whether it actually fixes a regression worth fast-tracking into v2.34.0 or not.

Ciao, Johannes

Show 7 quoted lines
>
>  	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>  	test_cmp expect args &&
> --
> 2.34.0.rc2.791.gdbfcf909579
>
>
Ævar Arnfjörð Bjarmason· Nov 10, 2021, 16:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

On Wed, Nov 10 2021, Johannes Schindelin wrote:
Show 13 quoted lines
> Hi Ævar,
>
> On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> Fix tests added in b681b191f92 (maintenance: add support for systemd
>> timers on Linux, 2021-09-04) to run successfully on systems where
>> systemd-analyze is installed, but on which there's a discrepancy
>> between a FILE argument of "/lib/systemd/system/basic.target" and
>> "systemd/user/git-maintenance@.service" succeeding.
>
> Could you try to rephrase `there's a discrepancy between a FILE argument
> of "/lib/systemd/system/basic.target" and
> "systemd/user/git-maintenance@.service" succeeding` more clearly?

Sure. Briefly to test if X works our prereq should test if X can work, not if Y can work. Will try to update it.

> Also, commit messages in git.git should not assume that every reader has a
> profound knowledge of `systemd`. The commit message needs to do a better
> job at explaining what is broken in the first place.

If I need to explain that in any detail I don't think this can go forward.

I don't really understand systemd well enough and don't have time to do so before the release.

This change seems to work well enough, and fixes the issue. What's *really* going on with systemd here, is there a better way to do this? I have no idea.

> The CI runs pass, after all, so it is unclear that there is anything
> broken that would need fixing to begin with.

Huh? It's unclear that we've got a portability-related breakage in git.git because CI passes?

The CI environment is a fairly monolithic environment that tests some very common setups. It's useful as a basic testing canary, but it's pretty much useless for finding out if we've got any sort of portability issues beyond a basic test/compile on linux/OSXWindows.

E.g. we routinely break builds and/or tests on the BSDs because of some Linux/Windows/OSX-specific assumptions in our code. CI will catch some of those, but not a large category of other issues.

So aside from this fix I don't think the GitHub CI can tell us much if anything in this regard.

Show 7 quoted lines
> [...]
>> We also need to pipe this output to FD's 5 & 6, to avoid mixing up the
>> trace output with our own output under "-x".
>
> We do not need to pipe to file descriptors 5 and 6. Other file descriptors
> would do, either. We need to redirect away from 1 and 2, is what this
> sentence should say.

Any other wouldn't do, because this is running in the context of test-lib.sh, which squat on some of the others from 3..9.

Yeah, I should mention that, but you can't just pick any of 3..9 here.
> And the hint about `-x` suggests that even that is not true that we need
> to redirect 1, either, just 2. And we would only need to redirect 2 with
> shells that do not understand `BASH_XTRACEFD`. It would be best not to
> mention `-x` at all.
I'd like both stdout & stderr here, and
    
    $ sh -c 'set -x; echo hi' >/dev/null 
    + echo hi
    $ sh -c 'set -x; echo hi' 2>/dev/null
    hi

I don't think we have a way in the tests to easily hoist the "turn off tracing" bit that test-lib.sh itself uses in similar cases, maybe I'm wrong and there's some easier way to do this.

Show 21 quoted lines
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> index 74aa6384755..5fe2ea03c1d 100755
>> --- a/t/t7900-maintenance.sh
>> +++ b/t/t7900-maintenance.sh
>> @@ -20,15 +20,16 @@ test_xmllint () {
>>  	fi
>>  }
>>
>> -test_lazy_prereq SYSTEMD_ANALYZE '
>> -	systemd-analyze verify /lib/systemd/system/basic.target
>> -'
>> -
>>  test_systemd_analyze_verify () {
>> -	if test_have_prereq SYSTEMD_ANALYZE
>> -	then
>> -		systemd-analyze verify "$@"
>> -	fi
>> +	# Ignoring any errors from systemd-analyze is intentional
>> +	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
>
> The semicolon is superfluous.
We use it in other places as shorthand for "I mean to not have && here".
> It is a bit sad that we're now doing unnecessary work when
> `systemd-analyze` does not even exist.

It's a chicken & egg problem. How do you figure out if you're able to run the command & get the output you expect without doing that?

Moving that to a test_have_prereq would just mean running it twice. Unless there's some better approach here I've missed.

Show 8 quoted lines
>> +
>> +	cat systemd.out >&5 &&
>> +	sed -n \
>> +		-e '/^Failed to load/d' \
>
> Lines starting with the prefix `Failed to load` are now deleted. Okay.
> Nothing in the commit explains why we can safely ignore those messages,
> though.
Quoting from it:
    
    To do that ignore whatever exit code we get from "systemd-analyze
    verify", and filter its stderr output to extract the sorts of lines it
    emits on note syntax warnings and errors. We need to filter out
    "Failed to load", which would be emitted e.g. on the
    gcc135.fsffrance.org test box[1].

Isn't that covered by that mention of "Failed to load"? Can you suggest a rewording you'd be happy with?

Show 7 quoted lines
>> +		-e '/git-maintenance@i*\.service:/x' \
>
> Lines containing `git-maintenance@.service:` (or the same pattern with an
> arbitrary number of `i`s after the `@` character???) are exchanged with
> hold space.
>
> That does not look right.

It'll emit @.service or @i.service. I have no idea why, yeah the regex is overly generous, but it doesn't hurt anything in this case (see below)>

> Since this is a `sed -n` call, we would need an explicit `p` command to
> print anything. Therefore, the current code is a pretty expensive
> equivalent to calling `true >&6`: it succeeds, and it does not print
> anything.
Yes, like the buggy "if the prereq succeeds" code it replaced.
Show 21 quoted lines
>> +		<systemd.err >&6 &&
>> +	rm systemd.out systemd.err
>>  }
>>
>>  test_expect_success 'help text' '
>> @@ -697,7 +698,11 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
>>  	# start registers the repo
>>  	git config --get --global --fixed-value maintenance.repo "$(pwd)" &&
>>
>> -	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" &&
>> +	# If we have a systemd-analyze on the system we can verify the
>> +	# generated file.
>> +	test_systemd_analyze_verify "systemd/user/git-maintenance@.service" 5>out 6>err &&
>> +	test_must_be_empty out &&
>> +	test_must_be_empty err &&
>
> Since the function name has the suffix `_verify`, the verification part
> should be _inside_ that function, not outside.
>
> This patch is not clear enough to tell whether it actually fixes a
> regression worth fast-tracking into v2.34.0 or not.

Because I really don't know. Is it broken on literally one machine in the world that I happened to have tested on, or more generally on the sorts of OS version/whatever that has that systemd? No idea.

But the worst we'll do here is not run a test on some obscure setup.

Since the value of having the test is there if we just run it on some setups that's fine. We should lean towards over-suppressing it to not have "make test" in v2.34.0 fail on some platforms.

I.e. as the commit message and the commits it links to explain we're really just asserting that when we change the systemd-specific code that we're not introducing syntax errors or whatever. So as long as this test runs for whoever is changing that (and they'd notice if it didn't) we're OK.

Junio C Hamano· Nov 11, 2021, 17:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2] maintenance tests: fix systemd v2.34.0-rc* test regression

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 18 quoted lines
> On Wed, Nov 10 2021, Johannes Schindelin wrote:
>
>> Hi Ævar,
>>
>> On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Fix tests added in b681b191f92 (maintenance: add support for systemd
>>> timers on Linux, 2021-09-04) to run successfully on systems where
>>> systemd-analyze is installed, but on which there's a discrepancy
>>> between a FILE argument of "/lib/systemd/system/basic.target" and
>>> "systemd/user/git-maintenance@.service" succeeding.
>>
>> Could you try to rephrase `there's a discrepancy between a FILE argument
>> of "/lib/systemd/system/basic.target" and
>> "systemd/user/git-maintenance@.service" succeeding` more clearly?
>
> Sure. Briefly to test if X works our prereq should test if X can work,
> not if Y can work. Will try to update it.
Thanks.
Show 11 quoted lines
>>>  test_systemd_analyze_verify () {
>>> -	if test_have_prereq SYSTEMD_ANALYZE
>>> -	then
>>> -		systemd-analyze verify "$@"
>>> -	fi
>>> +	# Ignoring any errors from systemd-analyze is intentional
>>> +	systemd-analyze verify "$@" >systemd.out 2>systemd.err;
>>
>> The semicolon is superfluous.
>
> We use it in other places as shorthand for "I mean to not have && here".

That is news to me (not the presense of unnecessary semicolons; I do not think we have "when you want to make it clear that you deliberately did not write &&, write ;" in any of the guidelines, and I am not sure if such a guideline is a good idea).

Show 5 quoted lines
>> It is a bit sad that we're now doing unnecessary work when
>> `systemd-analyze` does not even exist.
>
> It's a chicken & egg problem. How do you figure out if you're able to
> run the command & get the output you expect without doing that?

I read "It is a bit sad" to imply "... but it probalby is the best we can do, and more importantly, it is not worse than the problem it is solving".

Show 18 quoted lines
> +		-e '/git-maintenance@i*\.service:/x' \
>>
>> Lines containing `git-maintenance@.service:` (or the same pattern with an
>> arbitrary number of `i`s after the `@` character???) are exchanged with
>> hold space.
>>
>> That does not look right.
>
> It'll emit @.service or @i.service. I have no idea why, yeah the regex
> is overly generous, but it doesn't hurt anything in this case (see
> below)>
>
>> Since this is a `sed -n` call, we would need an explicit `p` command to
>> print anything. Therefore, the current code is a pretty expensive
>> equivalent to calling `true >&6`: it succeeds, and it does not print
>> anything.
>
> Yes, like the buggy "if the prereq succeeds" code it replaced.

If both are buggy, then there is no point replacing old with new. Let's have a non-buggy improved new and replace buggy old with it ;-)

Show 9 quoted lines
> Because I really don't know. Is it broken on literally one machine in
> the world that I happened to have tested on, or more generally on the
> sorts of OS version/whatever that has that systemd? No idea.
>
> But the worst we'll do here is not run a test on some obscure setup.
>
> Since the value of having the test is there if we just run it on some
> setups that's fine. We should lean towards over-suppressing it to not
> have "make test" in v2.34.0 fail on some platforms.

Hmph, if we _know_ (or _think_) the platforms that would fail a particular test is rare enough *and* if we know we _don't_ understand why the test fails on them, wouldn't we want to know the extent of the damage first by not suppressing the test and let it break on these platforms that may help us after the release to understand the breakage and deal with it? The resolution may end up to suppress the test after all, but with the approach we can do so knowing why we are doing so, no?

This is different from OpenSSH 8.7 thing that we _know_ why the test breaks, and we _know_ that there will be tons of instances with that broken version. Suppressing the test for them does make sense.

Thanks.
Ævar Arnfjörð Bjarmason· Oct 26, 2021, 11:15 UTC · re: Junio C Hamano · lore

pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Mon, Oct 25 2021, Junio C Hamano wrote:
> The fifteenth batch of topics are in 'master'.  I expect that this
> is more-or-less what we can expect in the -rc0, unless there is a
> hotfix to what's already merged.

I suggested a way forward for these iconv warnings that will be new in 2.34.0 at [1], poked'd a few days ago at [2].

Jeff: What do you think? Per [1] I think it's best to drop it entirely
for now, or split out just the "completely unknown encoding" problem
from "can't decode this particular thing".

You also had a patch in [3] that wasn't picked up, which would warn about this once.

If we *are* going to warn that seems like the worst of both in some sense, i.e. we'll no longer give users anything like "in this huge commit stream, we couldn't search in commit XYZ", instead we just warn on whatever happens to be the first commit, and you'll have no idea if subsequent matches were completed or not.

1. https://lore.kernel.org/git/871r4umfnm.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/211023.86sfwsis1i.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/YWEBmJk0aENR5Yeo@coredump.intra.peff.net/
Jeff King· Oct 27, 2021, 11:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Tue, Oct 26, 2021 at 01:15:00PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> 
> On Mon, Oct 25 2021, Junio C Hamano wrote:
> 
> > The fifteenth batch of topics are in 'master'.  I expect that this
> > is more-or-less what we can expect in the -rc0, unless there is a
> > hotfix to what's already merged.
> 
> I suggested a way forward for these iconv warnings that will be new in
> 2.34.0 at [1], poked'd a few days ago at [2].
Sorry to let this go for so long.

I do agree with your line of reasoning that it would be nice to sanity-check the incoming encoding once, but I think the details of doing so make things to tricky (to the point that it isn't worth doing; see my response just now in that thread).

Show 12 quoted lines
> Jeff: What do you think? Per [1] I think it's best to drop it entirely
> for now, or split out just the "completely unknown encoding" problem
> from "can't decode this particular thing".
> 
> You also had a patch in [3] that wasn't picked up, which would warn
> about this once.
> 
> If we *are* going to warn that seems like the worst of both in some
> sense, i.e. we'll no longer give users anything like "in this huge
> commit stream, we couldn't search in commit XYZ", instead we just warn
> on whatever happens to be the first commit, and you'll have no idea if
> subsequent matches were completed or not.
Yeah. I pretty much hate all of the options here. ;)

The firehose of warnings for "git log --encoding=nonsense" was known and discussed in fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). It's ugly for sure, but I'm still OK with it for the reasoning there: your next step is to fix the --encoding argument you gave. Whether you saw one line of warning or many is not that important, IMHO. Giving a single more sensible warning ("your encoding 'nonsense' isn't valid") would be better, but I think it's hard to do without creating other problems.

But the most compelling argument against warning at all is the case you gave earlier: that there may be historical garbage commits, and you can't get rid of them, so being warned constantly that we're not going to show or grep them correctly is just annoying. And that is true whether the user sees one warning or a hundred.

So while I do hate to have Git just silently ignore errors, probably the original behavior is the least-bad thing, and we should just revert fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). We probably want to salvage the documentation change (minus the "along with a warning") part.

Do you want to do the honors?
-Peff
Junio C Hamano· Oct 27, 2021, 18:04 UTC · re: Jeff King · lore

Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> The firehose of warnings for "git log --encoding=nonsense" was known and
> discussed in fd680bc558 (logmsg_reencode(): warn when iconv() fails,
> 2021-08-27). It's ugly for sure, but I'm still OK with it for the
> reasoning there: your next step is to fix the --encoding argument you
> gave. Whether you saw one line of warning or many is not that important,
> IMHO. Giving a single more sensible warning ("your encoding 'nonsense'
> isn't valid") would be better, but I think it's hard to do without
> creating other problems.
>
> But the most compelling argument against warning at all is the case you
> gave earlier: that there may be historical garbage commits, and you
> can't get rid of them, so being warned constantly that we're not going
> to show or grep them correctly is just annoying. And that is true
> whether the user sees one warning or a hundred.

Is it really a "firehose"? I won't use the word for one warning message per commit in the output of "git log --encoding=nonsense".

If you are running "git log --oneline", it may indeed be annoying to double the number of lines shown, and indeed

    $ git log --oneline --encoding=US-ASCII -4 ab/doc-lint
    warning: unable to reencode commit to 'US-ASCII'
    414abf159f docs: fix linting issues due to incorrect relative section order
    warning: unable to reencode commit to 'US-ASCII'
    ea8b9271b1 doc lint: lint relative section order
    warning: unable to reencode commit to 'US-ASCII'
    cafd9828e8 doc lint: lint and fix missing "GIT" end sections
    warning: unable to reencode commit to 'US-ASCII'
    d2c9908076 doc lint: fix bugs in, simplify and improve lint script

is indeed annoying, as everything that is _shown_ ought to be presentable in US-ASCII. This observation makes us realize an obvious approach to improve over the current behaviour without losing the warning when it matters, but I think the required code change, to first split the commit message into pieces (which roughly corresponds to the atoms in the --format= placeholder language) and reencode only these pieces that will be shown, may be too involved to be worth the effort.

Show 5 quoted lines
> So while I do hate to have Git just silently ignore errors, probably the
> original behavior is the least-bad thing, and we should just revert
> fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). We
> probably want to salvage the documentation change (minus the "along with
> a warning") part.

I am all for making it convenient to squelch, but it would be sad to lose the convenient way to notice possible misencoding in recent commits. Or can we have a command line option and pass it through the callchain, or would that be too involved?

Jeff King· Oct 28, 2021, 17:30 UTC · re: Junio C Hamano · lore

Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning

On Wed, Oct 27, 2021 at 11:04:50AM -0700, Junio C Hamano wrote:
Show 32 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > The firehose of warnings for "git log --encoding=nonsense" was known and
> > discussed in fd680bc558 (logmsg_reencode(): warn when iconv() fails,
> > 2021-08-27). It's ugly for sure, but I'm still OK with it for the
> > reasoning there: your next step is to fix the --encoding argument you
> > gave. Whether you saw one line of warning or many is not that important,
> > IMHO. Giving a single more sensible warning ("your encoding 'nonsense'
> > isn't valid") would be better, but I think it's hard to do without
> > creating other problems.
> >
> > But the most compelling argument against warning at all is the case you
> > gave earlier: that there may be historical garbage commits, and you
> > can't get rid of them, so being warned constantly that we're not going
> > to show or grep them correctly is just annoying. And that is true
> > whether the user sees one warning or a hundred.
> 
> Is it really a "firehose"?  I won't use the word for one warning
> message per commit in the output of "git log --encoding=nonsense".
> 
> If you are running "git log --oneline", it may indeed be annoying to
> double the number of lines shown, and indeed
> 
>     $ git log --oneline --encoding=US-ASCII -4 ab/doc-lint
>     warning: unable to reencode commit to 'US-ASCII'
>     414abf159f docs: fix linting issues due to incorrect relative section order
>     warning: unable to reencode commit to 'US-ASCII'
>     ea8b9271b1 doc lint: lint relative section order
>     warning: unable to reencode commit to 'US-ASCII'
>     cafd9828e8 doc lint: lint and fix missing "GIT" end sections
>     warning: unable to reencode commit to 'US-ASCII'
>     d2c9908076 doc lint: fix bugs in, simplify and improve lint script

It's a bit more than that. You get similar warnings for commits which we --grep but don't show (and which _might_ have been shown if the encoding conversion had been different). Try "git log --grep=foo --encoding=bar".

And of course the interleaved output you see above looks OK in a pager. But if you're sending the output of log (or diff-tree, etc) elsewhere, you're just going to get a stream of messages on stderr. That would be a bit less egregious if the message mentioned the commit oid, so they weren't strict duplicates.

Show 8 quoted lines
> is indeed annoying, as everything that is _shown_ ought to be
> presentable in US-ASCII.  This observation makes us realize an
> obvious approach to improve over the current behaviour without
> losing the warning when it matters, but I think the required code
> change, to first split the commit message into pieces (which roughly
> corresponds to the atoms in the --format= placeholder language) and
> reencode only these pieces that will be shown, may be too involved
> to be worth the effort.

Yeah, I think that would complicate things significantly, with the way the code is currently structured. It also means parsing commits that are in arbitrary encodings, which is not possible in most general sense. E.g., imagine an encoding which doesn't have ASCII as subset, like UTF-16. Though I suspect such encodings probably do not work for commits anyway (there is a chicken-and-egg with reading the encoding header in the first place).

Show 10 quoted lines
> > So while I do hate to have Git just silently ignore errors, probably the
> > original behavior is the least-bad thing, and we should just revert
> > fd680bc558 (logmsg_reencode(): warn when iconv() fails, 2021-08-27). We
> > probably want to salvage the documentation change (minus the "along with
> > a warning") part.
> 
> I am all for making it convenient to squelch, but it would be sad to
> lose the convenient way to notice possible misencoding in recent
> commits.  Or can we have a command line option and pass it through
> the callchain, or would that be too involved?

Do you mean a command-line option to squelch the warnings? I think it would not be too hard to do it as a config option (which is probably what you'd want anyway, since historical commits would come up over and over again).

-Peff
Junio C Hamano· Oct 28, 2021, 19:02 UTC · re: Jeff King · lore

Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
>> I am all for making it convenient to squelch, but it would be sad to
>> lose the convenient way to notice possible misencoding in recent
>> commits.  Or can we have a command line option and pass it through
>> the callchain, or would that be too involved?
>
> Do you mean a command-line option to squelch the warnings? I think it
> would not be too hard to do it as a config option (which is probably
> what you'd want anyway, since historical commits would come up over and
> over again).

Adding a "git -c please.be.verbose.on.encoding.errors=true" is sufficient for those who want to diagnose, but that is not very discoverable. Swapping the polarity and making it verbose by default, with a knob to squelch, may be more practical from that point of view.

Jeff King· Oct 28, 2021, 19:17 UTC · re: Junio C Hamano · lore

Re: pre-v2.34.0-rc0 regressions: 'git log' has a noisy iconv() warning

On Thu, Oct 28, 2021 at 12:02:23PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >> I am all for making it convenient to squelch, but it would be sad to
> >> lose the convenient way to notice possible misencoding in recent
> >> commits.  Or can we have a command line option and pass it through
> >> the callchain, or would that be too involved?
> >
> > Do you mean a command-line option to squelch the warnings? I think it
> > would not be too hard to do it as a config option (which is probably
> > what you'd want anyway, since historical commits would come up over and
> > over again).
> 
> Adding a "git -c please.be.verbose.on.encoding.errors=true" is
> sufficient for those who want to diagnose, but that is not very
> discoverable.  Swapping the polarity and making it verbose by
> default, with a knob to squelch, may be more practical from that
> point of view.

Right, it definitely has to be on by default or it will accomplish nothing. I was just asking about "config versus command-line".

And I suppose that does not really make implementation that much easier or harder. What makes it easy is if we accept a big global variable and do not have to pass it down through the call-stack.

-Peff
Ævar Arnfjörð Bjarmason· Oct 26, 2021, 12:13 UTC · re: Junio C Hamano · lore

tb/plug-pack-bitmap-leaks (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Mon, Oct 25 2021, Junio C Hamano wrote:
Show 14 quoted lines
> * tb/plug-pack-bitmap-leaks (2021-10-21) 9 commits
>  - pack-bitmap.c: more aggressively free in free_bitmap_index()
>  - pack-bitmap.c: don't leak type-level bitmaps
>  - pack-bitmap.c: avoid leaking via midx_bitmap_filename()
>  - builtin/multi-pack-index.c: don't leak concatenated options
>  - builtin/repack.c: avoid leaking child arguments
>  - builtin/pack-objects.c: don't leak memory via arguments
>  - t/helper/test-read-midx.c: free MIDX within read_midx_file()
>  - midx.c: don't leak MIDX from verify_midx_file
>  - midx.c: clean up chunkfile after reading the MIDX
>
>  Leakfix.
>
>  Will merge to 'next'?
These patches all look good to me.

I see you peeled off 10/11 and 11/11 from Taylor's submitted patches. The 10/11 re-submitted a patch that's in my ab/only-single-progress-at-once, and I really preferred 11/11 not going in, and instead suggested [1].

But since you've peeled off those two (I wouldn't have 10/11 at all) I think this is definitely ready for 'next'.

1. https://lore.kernel.org/git/patch-1.1-9190f3c128f-20211022T102725Z-avarab@gmail.com/
Taylor Blau· Oct 26, 2021, 21:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: tb/plug-pack-bitmap-leaks (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Tue, Oct 26, 2021 at 02:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 29 quoted lines
>
> On Mon, Oct 25 2021, Junio C Hamano wrote:
>
> > * tb/plug-pack-bitmap-leaks (2021-10-21) 9 commits
> >  - pack-bitmap.c: more aggressively free in free_bitmap_index()
> >  - pack-bitmap.c: don't leak type-level bitmaps
> >  - pack-bitmap.c: avoid leaking via midx_bitmap_filename()
> >  - builtin/multi-pack-index.c: don't leak concatenated options
> >  - builtin/repack.c: avoid leaking child arguments
> >  - builtin/pack-objects.c: don't leak memory via arguments
> >  - t/helper/test-read-midx.c: free MIDX within read_midx_file()
> >  - midx.c: don't leak MIDX from verify_midx_file
> >  - midx.c: clean up chunkfile after reading the MIDX
> >
> >  Leakfix.
> >
> >  Will merge to 'next'?
>
> These patches all look good to me.
>
> I see you peeled off 10/11 and 11/11 from Taylor's submitted
> patches. The 10/11 re-submitted a patch that's in my
> ab/only-single-progress-at-once, and I really preferred 11/11 not going
> in, and instead suggested [1].
>
> But since you've peeled off those two (I wouldn't have 10/11 at all) I
> think this is definitely ready for 'next'.
>
> 1. https://lore.kernel.org/git/patch-1.1-9190f3c128f-20211022T102725Z-avarab@gmail.com/

I sent a small updated version to fix a couple of things that I noticed during review here:

    https://lore.kernel.org/git/cover.1635282024.git.me@ttaylorr.com/T/#t

Using either version is fine, of course, but the one above may be a little nicer. (Apologies for the out-of-thread v2, I only noticed that I hadn't set `--in-reply-to` until after I had sent out the cover letter).

Thanks, Taylor

Ævar Arnfjörð Bjarmason· Oct 26, 2021, 12:17 UTC · re: Junio C Hamano · lore

jc/branch-copy-doc (was: What's cooking in git.git (Oct 2021, #06; Mon, 25))

On Mon, Oct 25 2021, Junio C Hamano wrote:
Show 7 quoted lines
> * jc/branch-copy-doc (2021-10-23) 1 commit
>  - branch (doc): -m/-c copies config and reflog
>
>  "git branch -c/-m new old" was not described to copy config, which
>  has been corrected.
>
>  Will merge to 'next'?
Looks correct & a good fix, thanks for clarifying that.
Derrick Stolee· Oct 26, 2021, 12:42 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On 10/25/2021 11:48 PM, Junio C Hamano wrote:
Show 15 quoted lines
> * vd/sparse-reset (2021-10-11) 8 commits
>  - unpack-trees: improve performance of next_cache_entry
>  - reset: make --mixed sparse-aware
>  - reset: make sparse-aware (except --mixed)
>  - reset: integrate with sparse index
>  - reset: expand test coverage for sparse checkouts
>  - sparse-index: update command for expand/collapse test
>  - reset: preserve skip-worktree bit in mixed reset
>  - reset: rename is_missing to !is_in_reset_tree
>  (this branch is used by ld/sparse-diff-blame.)
> 
>  Various operating modes of "git reset" have been made to work
>  better with the sparse index.
> 
>  Needs review.

This topic had good review in its first three versions, and the current v4 has had one response that doesn't seem to have actionable changes. Could you re-evaluate if the "needs review" label is still appropriate?

Thanks, -Stolee

Ævar Arnfjörð Bjarmason· Oct 26, 2021, 14:55 UTC · re: Derrick Stolee · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On Tue, Oct 26 2021, Derrick Stolee wrote:
Show 22 quoted lines
> On 10/25/2021 11:48 PM, Junio C Hamano wrote:
>
>> * vd/sparse-reset (2021-10-11) 8 commits
>>  - unpack-trees: improve performance of next_cache_entry
>>  - reset: make --mixed sparse-aware
>>  - reset: make sparse-aware (except --mixed)
>>  - reset: integrate with sparse index
>>  - reset: expand test coverage for sparse checkouts
>>  - sparse-index: update command for expand/collapse test
>>  - reset: preserve skip-worktree bit in mixed reset
>>  - reset: rename is_missing to !is_in_reset_tree
>>  (this branch is used by ld/sparse-diff-blame.)
>> 
>>  Various operating modes of "git reset" have been made to work
>>  better with the sparse index.
>> 
>>  Needs review.
>
> This topic had good review in its first three versions, and the
> current v4 has had one response that doesn't seem to have actionable
> changes. Could you re-evaluate if the "needs review" label is still
> appropriate?
[CC-ing Emily, the author of that one response]

Per [1] I think it should be "expecting a re-roll", unless I'm wrong about Victoria's "I can add that in my next version[...]" there, or missed some subsequent exchange not in that thread.

Or maybe it should be marked for "next", I haven't reviewed the latest version myself, just trying to help by filling in the gap about what the label should be, if not "needs review"...

1. https://lore.kernel.org/git/16fbc2dc-6fdd-ed0d-ebc6-3b0566142879@github.com
Victoria Dye· Oct 26, 2021, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

Ævar Arnfjörð Bjarmason wrote:
Show 32 quoted lines
> 
> On Tue, Oct 26 2021, Derrick Stolee wrote:
> 
>> On 10/25/2021 11:48 PM, Junio C Hamano wrote:
>>
>>> * vd/sparse-reset (2021-10-11) 8 commits
>>>  - unpack-trees: improve performance of next_cache_entry
>>>  - reset: make --mixed sparse-aware
>>>  - reset: make sparse-aware (except --mixed)
>>>  - reset: integrate with sparse index
>>>  - reset: expand test coverage for sparse checkouts
>>>  - sparse-index: update command for expand/collapse test
>>>  - reset: preserve skip-worktree bit in mixed reset
>>>  - reset: rename is_missing to !is_in_reset_tree
>>>  (this branch is used by ld/sparse-diff-blame.)
>>>
>>>  Various operating modes of "git reset" have been made to work
>>>  better with the sparse index.
>>>
>>>  Needs review.
>>
>> This topic had good review in its first three versions, and the
>> current v4 has had one response that doesn't seem to have actionable
>> changes. Could you re-evaluate if the "needs review" label is still
>> appropriate?
> 
> [CC-ing Emily, the author of that one response]
> 
> Per [1] I think it should be "expecting a re-roll", unless I'm wrong
> about Victoria's "I can add that in my next version[...]" there, or
> missed some subsequent exchange not in that thread.
> 

I am planning to re-roll with that test update, but I wanted to give it some time to see if other reviews came in. I'll send a new version tomorrow at the latest.

Show 6 quoted lines
> Or maybe it should be marked for "next", I haven't reviewed the latest
> version myself, just trying to help by filling in the gap about what the
> label should be, if not "needs review"...
> 
> 1. https://lore.kernel.org/git/16fbc2dc-6fdd-ed0d-ebc6-3b0566142879@github.com
>
Junio C Hamano· Oct 26, 2021, 21:29 UTC · re: Victoria Dye · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

Victoria Dye <vdye@github.com> writes:
Show 16 quoted lines
> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Oct 26 2021, Derrick Stolee wrote:
>> 
>>> On 10/25/2021 11:48 PM, Junio C Hamano wrote:
>>>
>>>> * vd/sparse-reset (2021-10-11) 8 commits
>>>> ...
>>>>  Various operating modes of "git reset" have been made to work
>>>>  better with the sparse index.
>>>>
>>>>  Needs review.
>
> I am planning to re-roll with that test update, but I wanted to give it some
> time to see if other reviews came in. I'll send a new version tomorrow at
> the latest.
Thanks.  Will mark it as "Expecting a reroll".
Junio C Hamano· Oct 26, 2021, 21:28 UTC · re: Derrick Stolee · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

Derrick Stolee <stolee@gmail.com> writes:
Show 9 quoted lines
>>  Various operating modes of "git reset" have been made to work
>>  better with the sparse index.
>> 
>>  Needs review.
>
> This topic had good review in its first three versions, and the
> current v4 has had one response that doesn't seem to have actionable
> changes. Could you re-evaluate if the "needs review" label is still
> appropriate?

The label indeed does not read well. I know there was a lot of dicussions on v3 but in the list traffic, but I do not see anybody looked at v4 and said that the issues they pointed out in v3 are now covered to their satisfaction. Probably "Needs ack" may have shown what I meant better?

Thanks.
Derrick Stolee· Oct 26, 2021, 21:54 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On 10/26/2021 5:28 PM, Junio C Hamano wrote:
Show 17 quoted lines
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>>  Various operating modes of "git reset" have been made to work
>>>  better with the sparse index.
>>>
>>>  Needs review.
>>
>> This topic had good review in its first three versions, and the
>> current v4 has had one response that doesn't seem to have actionable
>> changes. Could you re-evaluate if the "needs review" label is still
>> appropriate?
> 
> The label indeed does not read well.  I know there was a lot of
> dicussions on v3 but in the list traffic, but I do not see anybody
> looked at v4 and said that the issues they pointed out in v3 are now
> covered to their satisfaction.  Probably "Needs ack" may have shown
> what I meant better?

That makes sense. I think I've also seen labels such as "Is this ready?" before, which has a similar call to action.

And I'm sorry that I missed there was something to change in the test.

Thanks, -Stolee

Neeraj Singh· Oct 26, 2021, 22:21 UTC · re: Junio C Hamano · lore

regression in ns/tmp-objdir and ns/batched-fsync

On Mon, Oct 25, 2021 at 08:48:04PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> * ns/tmp-objdir (2021-10-04) 2 commits
>   (merged to 'next' on 2021-10-23 at 358d376f61)
>  + tmp-objdir: disable ref updates when replacing the primary odb
>  + tmp-objdir: new API for creating temporary writable databases
>  (this branch is used by ns/batched-fsync and ns/remerge-diff.)
> 
>  New interface into the tmp-objdir API to help in-core use of the
>  quarantine feature.
> 
>  Will merge to 'master'.
> 
--snip--
Show 17 quoted lines
> * ns/batched-fsync (2021-10-08) 8 commits
>   (merged to 'next' on 2021-10-25 at e45c907d41)
>  + core.fsyncobjectfiles: performance tests for add and stash
>  + core.fsyncobjectfiles: tests for batch mode
>  + unpack-objects: use the bulk-checkin infrastructure
>  + update-index: use the bulk-checkin infrastructure
>  + core.fsyncobjectfiles: add windows support for batch mode
>  + core.fsyncobjectfiles: batched disk flushes
>  + bulk-checkin: rename 'state' variable and separate 'plugged' boolean
>  + Merge branch 'ns/tmp-objdir' into ns/batched-fsync
>  (this branch uses ns/tmp-objdir.)
> 
>  The "core.fsyncobjectfiles" configuration variable can now be set
>  to "batch" for improved performance.
> 
>  Will merge to 'master'.
> 

Hi Junio, When DScho enabled batch fsync by default (for all platforms) in the git-for-windows tree, CI caught a regression. It came up through the heavy refactoring to use the tmp-objdir code.

The problem is related to setup_work_tree erasing the tmp_objdir "primary" odb.  Please expect a incremental patch from me shortly against ns/tmp-objdir. It will need to be merged into ns/batched-fsync.

Thanks, Neeraj

Martin Ågren· Oct 27, 2021, 19:17 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

On Tue, 26 Oct 2021 at 09:00, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> * ma/doc-git-version (2021-10-25) 1 commit
>   (merged to 'next' on 2021-10-25 at 9f74afec0c)
>  + git.txt: fix typo
>
>  Typofix.
>
>  Will merge to 'master'.

I notice you didn't pick up [1] which was posted as part of the same mini-series as this one. Maybe you intended to place it on ab/unbundle-progress and got distracted. Or if you don't agree with the patch, that's also fine. :-) Just checking.

[1] https://lore.kernel.org/git/7defb11c1c295550105a47bfdd9f030e0ef7c636.1635093692.git.martin.agren@gmail.com/
Martin
Junio C Hamano· Oct 28, 2021, 00:06 UTC · re: Martin Ågren · lore

Re: What's cooking in git.git (Oct 2021, #06; Mon, 25)

Martin Ågren <martin.agren@gmail.com> writes:
Show 13 quoted lines
> On Tue, 26 Oct 2021 at 09:00, Junio C Hamano <gitster@pobox.com> wrote:
>> * ma/doc-git-version (2021-10-25) 1 commit
>>   (merged to 'next' on 2021-10-25 at 9f74afec0c)
>>  + git.txt: fix typo
>>
>>  Typofix.
>>
>>  Will merge to 'master'.
>
> I notice you didn't pick up [1] which was posted as part of the same
> mini-series as this one. Maybe you intended to place it on
> ab/unbundle-progress and got distracted. Or if you don't agree with the
> patch, that's also fine. :-) Just checking.

Ah, that is a good one. Yes, I noticed that these two unrelated changes were sent as if they belong to the same topic, and I meant to later find out which branch that other one should go, but was distracted.

Thanks.

← back to recent threads