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

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

[Junio C Hamano]

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.

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

[Jeff King]

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.

-- >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

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))

[Ævar Arnfjörð Bjarmason]

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/

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))

[Ævar Arnfjörð Bjarmason]

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/

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

[Ævar Arnfjörð Bjarmason]

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/

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

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 25 2021, Junio C Hamano wrote:

> * 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.

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

[Derrick Stolee]

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?

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

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.

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

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))

[Ævar Arnfjörð Bjarmason]

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

> 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.

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

[Victoria Dye]

Æ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
>>>  - 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.

> 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
>

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))

[Eric Sunshine]

On Tue, Oct 26, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> [...]
> 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...)

> 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.

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

[Taylor Blau]

On Tue, Oct 26, 2021 at 02:13:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> 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

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

[Junio C Hamano]

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?

Thanks.

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

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> Æ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".

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

[Derrick Stolee]

On 10/26/2021 5:28 PM, Junio C Hamano wrote:
> 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

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

[Neeraj Singh]

On Mon, Oct 25, 2021 at 08:48:04PM -0700, Junio C Hamano wrote:
> * 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--

> * 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

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))

[Jeff King]

On Tue, Oct 26, 2021 at 01:15:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 
> 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).

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Junio C Hamano]

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

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.

> 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?

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

[Martin Ågren]

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.

[1] https://lore.kernel.org/git/7defb11c1c295550105a47bfdd9f030e0ef7c636.1635093692.git.martin.agren@gmail.com/

Martin

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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.

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

[Jeff King]

On Wed, Oct 27, 2021 at 11:04:50AM -0700, Junio C Hamano wrote:

> 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.

> 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).

> > 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Thu, Oct 28, 2021 at 12:02:23PM -0700, Junio C Hamano wrote:

> 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

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

[Kaartic Sivaraam]

On 26/10/21 10:55 am, Jeff King wrote:
> 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

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

[Jeff King]

On Mon, Nov 01, 2021 at 12:06:20AM +0530, Kaartic Sivaraam wrote:

> > 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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Eric Sunshine]

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/

> 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/


> "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>

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

[Ævar Arnfjörð Bjarmason]

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]

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?

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.

> 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.

> 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.

> 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.

> +		<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

>
>  	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
>  	test_cmp expect args &&
> --
> 2.34.0.rc2.791.gdbfcf909579
>
>

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

[Ævar Arnfjörð Bjarmason]

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.

> 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.

> [...]
>> 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.

>> 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.

>> +
>> +	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?

>> +		-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.

>> +		<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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.

>>>  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).

>> 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".

> +		-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 ;-)

> 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.