[ANNOUNCE] Git v2.34.0-rc2

[ANNOUNCE] Git v2.34.0-rc2

[Junio C Hamano]

A release candidate Git v2.34.0-rc2 is now available for testing at
the usual places.  It is comprised of 786 non-merge commits since
v2.33.0, contributed by 93 people, 28 of which are new faces [*].

The tarballs are found at:

    https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.34.0-rc2' tag and the 'master' branch that the tag points at:

  url = https://git.kernel.org/pub/scm/git/git
  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.33.0 are as follows.
Welcome to the Git development community!

  Alan Blotz, Azeem Bande-Ali, Dr. Matthias St. Pierre, Eli
  Schwartz, git.mexon@spamgourmet.com, Glen Choo, Hamza Mahfooz,
  Joel Klinghed, Johannes Altmanninger, Jonas Kittner, Kim
  Altintop, Kyle Zhao, Mahi Kolla, Marvin Häuser, Mickey Endito,
  Rob Browning, Robert Estelle, Robert Leftwich, Robin Dupret,
  Tal Kelrich, Tassilo Horn, Thomas De Zeeuw, USAMI Kenta, Victor
  Gambier, Victoria Dye, Wesley Schwengle, Xingman Chen, and Zoker.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Adam Dinwoodie, Ævar Arnfjörð Bjarmason, Alban Gruin, Alex
  Henrie, Alex Riesen, Andrzej Hunt, Atharva Raykar, Bagas Sanjaya,
  Ben Boeckel, brian m. carlson, Carlo Arenas, Carlo Marcelo
  Arenas Belón, Christian Couder, David Aguilar, David Turner,
  Derrick Stolee, Đoàn Trần Công Danh, Elijah Newren, Emily
  Shaffer, Emir Sarı, Eric Sunshine, Eric Wong, Fabian Stelzer,
  Felipe Contreras, Greg Hurrell, Han-Wen Nienhuys, Hariom Verma,
  Jacob Keller, Jacob Vosmaer, Jean-Noël Avila, Jeff Hostetler,
  Jeff King, Jiang Xin, Johannes Schindelin, Johannes Sixt,
  Jonathan Nieder, Jonathan Tan, Josh Steadmon, Junio C Hamano,
  Kaartic Sivaraam, Lénaïc Huard, Martin Ågren, Matheus Tavares,
  Matheus Tavares Bernardino, Matthias Aßhauer, Mike Hommey,
  Miriam Rubio, Orgad Shaneh, Patrick Steinhardt, Philip Oakley,
  Philippe Blain, Phillip Wood, Pranit Bauva, Randall S. Becker,
  René Scharfe, Sergey Organov, Shourya Shukla, SZEDER Gábor,
  Takashi Iwai, Tanushree Tumane, Taylor Blau, Teng Long, Todd
  Zullinger, Ulrich Windl, and ZheNing Hu.

[*] We are counting not just the authorship contribution but issue
    reporting, mentoring, helping and reviewing that are recorded in
    the commit trailers.

----------------------------------------------------------------

Git 2.34 Release Notes (draft)
==============================

Updates since Git 2.33
----------------------

Backward compatibility notes

 * The "--preserve-merges" option of "git rebase" has been removed.


UI, Workflows & Features

 * Pathname expansion (like "~username/") learned a way to specify a
   location relative to Git installation (e.g. its $sharedir which is
   $(prefix)/share), with "%(prefix)".

 * Use `ort` instead of `recursive` as the default merge strategy.

 * The userdiff pattern for "java" language has been updated.

 * "git rebase" by default skips changes that are equivalent to
   commits that are already in the history the branch is rebased onto;
   give messages when this happens to let the users be aware of
   skipped commits, and also teach them how to tell "rebase" to keep
   duplicated changes.

 * The advice message that "git cherry-pick" gives when it asks
   conflicted replay of a commit to be resolved by the end user has
   been updated.

 * After "git clone --recurse-submodules", all submodules are cloned
   but they are not by default recursed into by other commands.  With
   submodule.stickyRecursiveClone configuration set, submodule.recurse
   configuration is set to true in a repository created by "clone"
   with "--recurse-submodules" option.

 * The logic for auto-correction of misspelt subcommands learned to go
   interactive when the help.autocorrect configuration variable is set
   to 'prompt'.

 * "git maintenance" scheduler learned to use systemd timers as a
   possible backend.

 * "git diff --submodule=diff" showed failure from run_command() when
   trying to run diff inside a submodule, when the user manually
   removes the submodule directory.

 * "git bundle unbundle" learned to show progress display.

 * In cone mode, the sparse-index code path learned to remove ignored
   files (like build artifacts) outside the sparse cone, allowing the
   entire directory outside the sparse cone to be removed, which is
   especially useful when the sparse patterns change.

 * Taking advantage of the CGI interface, http-backend has been
   updated to enable protocol v2 automatically when the other side
   asks for it.

 * The credential-cache helper has been adjusted to Windows.

 * The error in "git help no-such-git-command" is handled better.

 * The unicode character width table (used for output alignment) has
   been updated.

 * The ref iteration code used to optionally allow dangling refs to be
   shown, which has been tightened up.

 * "git add", "git mv", and "git rm" have been adjusted to avoid
   updating paths outside of the sparse-checkout definition unless
   the user specifies a "--sparse" option.

 * "git repack" has been taught to generate multi-pack reachability
   bitmaps.

 * "git fsck" has been taught to report mismatch between expected and
   actual types of an object better.

 * Use ssh public crypto for object and push-cert signing.

 * "git log --grep=string --author=name" learns to highlight hits just
   like "git grep string" does.



Performance, Internal Implementation, Development Support etc.

 * "git bisect" spawned "git show-branch" only to pretty-print the
   title of the commit after checking out the next version to be
   tested; this has been rewritten in C.

 * "git add" can work better with the sparse index.

 * Support for ancient versions of cURL library (pre 7.19.4) has been
   dropped.

 * A handful of tests that assumed implementation details of files
   backend for refs have been cleaned up.

 * trace2 logs learned to show parent process name to see in what
   context Git was invoked.

 * Loading of ref tips to prepare for common ancestry negotiation in
   "git fetch-pack" has been optimized by taking advantage of the
   commit graph when available.

 * Remind developers that the userdiff patterns should be kept simple
   and permissive, assuming that the contents they apply are always
   syntactically correct.

 * The current implementation of GIT_TEST_FAIL_PREREQS is broken in
   that checking for the lack of a prerequisite would not work.  Avoid
   the use of "if ! test_have_prereq X" in a test script.

 * The revision traversal API has been optimized by taking advantage
   of the commit-graph, when available, to determine if a commit is
   reachable from any of the existing refs.

 * "git fetch --quiet" optimization to avoid useless computation of
   info that will never be displayed.

 * Callers from older advice_config[] based API has been updated to
   use the newer advice_if_enabled() and advice_enabled() API.

 * Teach "test_pause" and "debug" helpers to allow using the HOME and
   TERM environment variables the user usually uses.

 * "make INSTALL_STRIP=-s install" allows the installation step to use
   "install -s" to strip the binaries as they get installed.

 * Code that handles large number of refs in the "git fetch" code
   path has been optimized.

 * The reachability bitmap file used to be generated only for a single
   pack, but now we've learned to generate bitmaps for history that
   span across multiple packfiles.

 * The code to make "git grep" recurse into submodules has been
   updated to migrate away from the "add submodule's object store as
   an alternate object store" mechanism (which is suboptimal).

 * The tracing of process ancestry information has been enhanced.

 * Reduce number of write(2) system calls while sending the
   ref advertisement.

 * Update the build procedure to use the "-pedantic" build when
   DEVELOPER makefile macro is in effect.

 * Large part of "git submodule add" gets rewritten in C.

 * The run-command API has been updated so that the callers can easily
   ask the file descriptors open for packfiles to be closed immediately
   before spawning commands that may trigger auto-gc.

 * An oddball OPTION_ARGUMENT feature has been removed from the
   parse-options API.

 * The mergesort implementation used to sort linked list has been
   optimized.

 * Remove external declaration of functions that no longer exist.

 * "git multi-pack-index write --bitmap" learns to propagate the
   hashcache from original bitmap to resulting bitmap.

 * CI learns to run the leak sanitizer builds.

 * "git grep --recurse-submodules" takes trees and blobs from the
   submodule repository, but the textconv settings when processing a
   blob from the submodule is not taken from the submodule repository.
   A test is added to demonstrate the issue, without fixing it.

 * Teach "git help -c" into helping the command line completion of
   configuration variables.

 * When "git cmd -h" shows more than one line of usage text (e.g.
   the cmd subcommand may take sub-sub-command), parse-options API
   learned to align these lines, even across i18n/l10n.

 * Prevent "make sparse" from running for the source files that
   haven't been modified.

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

 * A new feature has been added to abort early in the test framework.


Fixes since v2.33
-----------------

 * Input validation of "git pack-objects --stdin-packs" has been
   corrected.

 * Bugfix for common ancestor negotiation recently introduced in "git
   push" code path.

 * "git pull" had various corner cases that were not well thought out
   around its --rebase backend, e.g. "git pull --ff-only" did not stop
   but went ahead and rebased when the history on other side is not a
   descendant of our history.  The series tries to fix them up.

 * "git apply" miscounted the bytes and failed to read to the end of
   binary hunks.

 * "git range-diff" code clean-up.

 * "git commit --fixup" now works with "--edit" again, after it was
   broken in v2.32.

 * Use upload-artifacts v1 (instead of v2) for 32-bit linux, as the
   new version has a blocker bug for that architecture.

 * Checking out all the paths from HEAD during the last conflicted
   step in "git rebase" and continuing would cause the step to be
   skipped (which is expected), but leaves MERGE_MSG file behind in
   $GIT_DIR and confuses the next "git commit", which has been
   corrected.

 * Various bugs in "git rebase -r" have been fixed.

 * mmap() imitation used to call xmalloc() that dies upon malloc()
   failure, which has been corrected to just return an error to the
   caller to be handled.

 * "git diff --relative" segfaulted and/or produced incorrect result
   when there are unmerged paths.

 * The delayed checkout code path in "git checkout" etc. were chatty
   even when --quiet and/or --no-progress options were given.

 * "git branch -D <branch>" used to refuse to remove a broken branch
   ref that points at a missing commit, which has been corrected.

 * Build update for Apple clang.

 * The parser for the "--nl" option of "git column" has been
   corrected.

 * "git upload-pack" which runs on the other side of "git fetch"
   forgot to take the ref namespaces into account when handling
   want-ref requests.

 * The sparse-index support can corrupt the index structure by storing
   a stale and/or uninitialized data, which has been corrected.

 * Buggy tests could damage repositories outside the throw-away test
   area we created.  We now by default export GIT_CEILING_DIRECTORIES
   to limit the damage from such a stray test.

 * Even when running "git send-email" without its own threaded
   discussion support, a threading related header in one message is
   carried over to the subsequent message to result in an unwanted
   threading, which has been corrected.

 * The output from "git fast-export", when its anonymization feature
   is in use, showed an annotated tag incorrectly.

 * Doc update plus improved error reporting.

 * Recent "diff -m" changes broke "gitk", which has been corrected.

 * Regression fix.

 * The "git apply -3" code path learned not to bother the lower level
   merge machinery when the three-way merge can be trivially resolved
   without the content level merge.  This fixes a regression caused by
   recent "-3way first and fall back to direct application" change.

 * The code that optionally creates the *.rev reverse index file has
   been optimized to avoid needless computation when it is not writing
   the file out.

 * "git range-diff -I... <range> <range>" segfaulted, which has been
   corrected.

 * The order in which various files that make up a single (conceptual)
   packfile has been reevaluated and straightened up.  This matters in
   correctness, as an incomplete set of files must not be shown to a
   running Git.

 * The "mode" word is useless in a call to open(2) that does not
   create a new file.  Such a call in the files backend of the ref
   subsystem has been cleaned up.

 * "git update-ref --stdin" failed to flush its output as needed,
   which potentially led the conversation to a deadlock.

 * When "git am --abort" fails to abort correctly, it still exited
   with exit status of 0, which has been corrected.

 * Correct nr and alloc members of strvec struct to be of type size_t.

 * "git stash", where the tentative change involves changing a
   directory to a file (or vice versa), was confused, which has been
   corrected.

 * "git clone" from a repository whose HEAD is unborn into a bare
   repository didn't follow the branch name the other side used, which
   is corrected.

 * "git cvsserver" had a long-standing bug in its authentication code,
   which has finally been corrected (it is unclear and is a separate
   question if anybody is seriously using it, though).

 * "git difftool --dir-diff" mishandled symbolic links.

 * Sensitive data in the HTTP trace were supposed to be redacted, but
   we failed to do so in HTTP/2 requests.

 * "make clean" has been updated to remove leftover .depend/
   directories, even when it is not told to use them to compute header
   dependencies.

 * Protocol v0 clients can get stuck parsing a malformed feature line.

 * A few kinds of changes "git status" can show were not documented.
   (merge d2a534c515 ja/doc-status-types-and-copies later to maint).

 * The mergesort implementation used to sort linked list has been
   optimized.
   (merge c90cfc225b rs/mergesort later to maint).

 * An editor session launched during a Git operation (e.g. during 'git
   commit') can leave the terminal in a funny state.  The code path
   has updated to save the terminal state before, and restore it
   after, it spawns an editor.
   (merge 3d411afabc cm/save-restore-terminal later to maint).

 * "git cat-file --batch" with the "--batch-all-objects" option is
   supposed to iterate over all the objects found in a repository, but
   it used to translate these object names using the replace mechanism,
   which defeats the point of enumerating all objects in the repository.
   This has been corrected.
   (merge bf972896d7 jk/cat-file-batch-all-wo-replace later to maint).

 * Recent sparse-index work broke safety against attempts to add paths
   with trailing slashes to the index, which has been corrected.
   (merge c8ad9d04c6 rs/make-verify-path-really-verify-again later to maint).

 * The "--color-lines" and "--color-by-age" options of "git blame"
   have been missing, which are now documented.
   (merge 8c32856133 bs/doc-blame-color-lines later to maint).

 * The PATH used in CI job may be too wide and let incompatible dlls
   to be grabbed, which can cause the build&test to fail.  Tighten it.
   (merge 7491ef6198 js/windows-ci-path-fix later to maint).

 * Avoid performance measurements from getting ruined by gc and other
   housekeeping pauses interfering in the middle.
   (merge be79131a53 rs/disable-gc-during-perf-tests later to maint).

 * Stop "git add --dry-run" from creating new blob and tree objects.
   (merge e578d0311d rs/add-dry-run-without-objects later to maint).

 * "git commit" gave duplicated error message when the object store
   was unwritable, which has been corrected.
   (merge 4ef91a2d79 ab/fix-commit-error-message-upon-unwritable-object-store later to maint).

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

 * 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.
   (merge 571f4348dd da/mergetools-special-case-xxdiff-exit-128 later to maint).

 * Improve test framework around unwritable directories.
   (merge 5d22e18965 ab/test-cleanly-recreate-trash-directory later to maint).

 * "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.
   (merge c5c3486f38 jk/http-push-status-fix later to maint).

 * Update "git archive" documentation and give explicit mention on the
   compression level for both zip and tar.gz format.
   (merge c4b208c309 bs/archive-doc-compression-level later to maint).

 * Drop "git sparse-checkout" from the list of common commands.
   (merge 6a9a50a8af sg/sparse-index-not-that-common-a-command later to maint).

 * "git branch -c/-m new old" was not described to copy config, which
   has been corrected.
   (merge 8252ec300e jc/branch-copy-doc later to maint).

 * Squelch over-eager warning message added during this cycle.
   (merge 9e8fe7b1c7 jk/log-warn-on-bogus-encoding later to maint).

 * Fix long-standing shell syntax error in the completion script.
   (merge 46b0585286 re/completion-fix-test-equality later to maint).

 * 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.
   (merge 095d112f8c ab/ignore-replace-while-working-on-commit-graph later to maint).

 * "git pull --no-verify" did not affect the underlying "git merge".
   (merge 47bfdfb3fd ar/fix-git-pull-no-verify later to maint).

 * One CI task based on Fedora image noticed a not-quite-kosher
   consturct recently, which has been corrected.
   (merge 4b540cf913 vd/pthread-setspecific-g11-fix later to maint).

 * Other code cleanup, docfix, build fix, etc.
   (merge f188160be9 ab/bundle-remove-verbose-option later to maint).
   (merge 8c6b4332b4 rs/close-pack-leakfix later to maint).
   (merge 51b04c05b7 bs/difftool-msg-tweak later to maint).
   (merge dd20e4a6db ab/make-compdb-fix later to maint).
   (merge 6ffb990dc4 os/status-docfix later to maint).
   (merge 100c2da2d3 rs/p3400-lose-tac later to maint).
   (merge 76f3b69896 tb/aggregate-ignore-leading-whitespaces later to maint).
   (merge 6e4fd8bfcd tz/doc-link-to-bundle-format-fix later to maint).
   (merge f6c013dfa1 jc/doc-commit-header-continuation-line later to maint).
   (merge ec9a37d69b ab/pkt-line-cleanup later to maint).
   (merge 8650c6298c ab/fix-make-lint-docs later to maint).
   (merge 1c720357ce ab/test-lib-diff-cleanup later to maint).
   (merge 6b615dbece ks/submodule-add-message-fix later to maint).
   (merge 82a57cd13f ma/doc-git-version later to maint).
   (merge 203eb8381a jc/doc-format-patch-clarify-auto-base later to maint).
   (merge 559664c792 ab/test-lib later to maint).

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:

>  * Use ssh public crypto for object and push-cert signing.

I'm seeing some test breakage from the release candidates here. On my
Debian unstable system, everything passed a few days ago. But after
upgrading openssh-client from 1:8.4p1-5 to 1:8.7p1-1 (which hit unstable
on Saturday), all of the GPGSSH bits seem to break:

  Test Summary Report
  -------------------
  t5534-push-signed.sh                             (Wstat: 256 Tests: 13 Failed: 2)
    Failed tests:  8, 12
    Non-zero exit status: 1
  t6200-fmt-merge-msg.sh                           (Wstat: 256 Tests: 31 Failed: 2)
    Failed tests:  7-8
    Non-zero exit status: 1
  t7031-verify-tag-signed-ssh.sh                   (Wstat: 256 Tests: 8 Failed: 5)
    Failed tests:  2, 4-7
    Non-zero exit status: 1
  t7528-signed-commit-ssh.sh                       (Wstat: 256 Tests: 23 Failed: 10)
    Failed tests:  2-5, 7, 9, 12-13, 17, 19
    Non-zero exit status: 1

This doesn't have anything to do with -rc2 in particular. The breakage
bisects to f265f2d630 (ssh signing: tests for logs, tags & push certs,
2021-09-10), and is triggered by the system openssh upgrade.

It's hard to tell what's going on, as we seem to just be getting bad
results from ssh-keygen. Here's the first failing test in t7031 (with
GIT_TRACE and -x):

  [...]
  + git verify-tag initial
  trace: built-in: git verify-tag initial
  trace: run_command: ssh-keygen -Y find-principals -f '/home/peff/compile/git/t/trash directory.t7031-verify-tag-signed-ssh/gpghome/ssh.all_valid.allowedSignersFile' -s /tmp/.git_vtag_tmpSxXLIv
  trace: run_command: ssh-keygen -Y check-novalidate -n git -s /tmp/.git_vtag_tmpSxXLIv
  + exit 1
  error: last command exited with $?=1
  not ok 2 - verify and show ssh signatures

Likewise, this segfault (!) from t7528 is scary:

  [...]
  + git verify-commit initial
  trace: built-in: git verify-commit initial
  trace: run_command: ssh-keygen -Y find-principals -f '/home/peff/compile/git/t/trash directory.t7528-signed-commit-ssh/gpghome/ssh.all_valid.allowedSignersFile' -s /tmp/.git_vtag_tmpCOAwhp
  error: ssh-keygen died of signal 11
  trace: run_command: ssh-keygen -Y check-novalidate -n git -s /tmp/.git_vtag_tmpCOAwhp
  Good "git" signature with ED25519 key SHA256:E+1Xptv1zGa2fWFjSL36Tl2m2NVxcyJVzhfQTnU+yWc
  + exit 1
  error: last command exited with $?=1
  not ok 2 - verify and show signatures

So it may not be a bug we need to fix in Git. But shipping v2.34 with
lots of test failures may cause some headaches. Maybe we need to tighten
up the GPGSSH prereq checks to block broken versions?

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Wed, Nov 10, 2021 at 12:41:11AM -0500, Jeff King wrote:

> So it may not be a bug we need to fix in Git. But shipping v2.34 with
> lots of test failures may cause some headaches. Maybe we need to tighten
> up the GPGSSH prereq checks to block broken versions?

This is what I came up with, but I'm not sure it there's a better way to
find the broken version. I don't think there's a way to get a version
number out of ssh-keygen (and anyway, checking the behavior of the
command is a more robust test). My fears are:

  - this does cause several segfaults per test run on affected
    platforms, which will pollute the kernel log, etc.

  - we're not really testing the desired behavior, just looking for a
    known-problem. The segfault may get fixed but we'd still have other
    bugs.

So it would be nice to have a more exact test, but without understanding
the openssh bug, I think this is the best we can do in the meantime.

-- >8 --
Subject: [PATCH] t/lib-gpg: avoid broken versions of ssh-keygen

The "-Y find-principals" option of ssh-keygen seems to be broken in
Debian's openssh-client 1:8.7p1-1, whereas it works fine in 1:8.4p1-5.
This causes several failures for GPGSSH tests. We fulfill the
prerequisite because generating the keys works fine, but actually
verifying a signature causes results ranging from bogus results to
ssh-keygen segfaulting.

We can find the broken version during the prereq check by feeding it
empty input. This should result in it complaining to stderr, but in the
broken version it triggers the segfault, causing the GPGSSH tests to be
skipped.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-gpg.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 1d8e5b5b7e..a3f285f515 100644
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -104,6 +104,12 @@ test_lazy_prereq GPGSSH '
 	test $? != 127 || exit 1
 	echo $ssh_version | grep -q "find-principals:missing signature file"
 	test $? = 0 || exit 1;
+
+	# some broken versions of ssh-keygen segfault on find-principals;
+	# avoid testing with them.
+	ssh-keygen -Y find-principals -f /dev/null -s /dev/null
+	test $? = 139 && exit 1
+
 	mkdir -p "${GNUPGHOME}" &&
 	chmod 0700 "${GNUPGHOME}" &&
 	(setfacl -k "${GNUPGHOME}" 2>/dev/null || true) &&
-- 
2.34.0.rc1.634.g85d556ea55

Re: [ANNOUNCE] Git v2.34.0-rc2

[Johannes Altmanninger]

On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:
> A release candidate Git v2.34.0-rc2 is now available for testing
> 
> Fixes since v2.33
> -----------------
> 
>  * Doc update plus improved error reporting.

This should be something like

 * Warn when iconv(3) fails to reencode log messages.

> 
>  * Recent "diff -m" changes broke "gitk", which has been corrected.
> 
>  * Regression fix.

I assume that generic merge messages like "Regression fix." or "Doc update."
are meant to be dropped or replaced, maybe

  * Fix a v2.33.0 regression where "git send-email" would use wrong values for
    sendemail.* options that are defined in multiple configuration files.

>  * One CI task based on Fedora image noticed a not-quite-kosher
>    consturct recently, which has been corrected.

I guess consturct -> construct

>    (merge 4b540cf913 vd/pthread-setspecific-g11-fix later to maint).

Re: [ANNOUNCE] Git v2.34.0-rc2

[Carlo Arenas]

On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:

>   - we're not really testing the desired behavior, just looking for a
>     known-problem. The segfault may get fixed but we'd still have other
>     bugs.

This openssh bug was fixed in 8.8 per the release notes; indeed the
fix[1] (which was misapplied but fixed next commit) looks familiar and
it is just a straight up crasher, hence unlikely to cause other
issues.

Agree with you though that with both git and openssh getting changes
released for this feature there is a probability that there might be
misbehaving combinations that are not appropriately tested, but your
patch is a good step in the right direction.

Carlo

[1] https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/ssh-keygen.c.diff?r1=1.435&r2=1.436&f=h

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:

> On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:
> 
> >   - we're not really testing the desired behavior, just looking for a
> >     known-problem. The segfault may get fixed but we'd still have other
> >     bugs.
> 
> This openssh bug was fixed in 8.8 per the release notes; indeed the
> fix[1] (which was misapplied but fixed next commit) looks familiar and
> it is just a straight up crasher, hence unlikely to cause other
> issues.

Ah, thanks for digging. I agree that this is a small isolated bug, so
the prereq check I showed would be a good test for it.

IMHO it's worth doing. It looks like 8.7 is the only affected openssh
version, but it is likely to cause confusion right when we release.

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Wed, Nov 10, 2021 at 07:35:47AM +0100, Johannes Altmanninger wrote:

> > Fixes since v2.33
> > -----------------
> > 
> >  * Doc update plus improved error reporting.
> 
> This should be something like
> 
>  * Warn when iconv(3) fails to reencode log messages.

Yes, though note that we ended up reverting the warning. So probably it
could be removed from the release notes entirely.

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Carlo Arenas]

On Wed, Nov 10, 2021 at 12:22 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:
>
> > On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:
> >
> > >   - we're not really testing the desired behavior, just looking for a
> > >     known-problem. The segfault may get fixed but we'd still have other
> > >     bugs.
> >
> > This openssh bug was fixed in 8.8 per the release notes; indeed the
> > fix[1] (which was misapplied but fixed next commit) looks familiar and
> > it is just a straight up crasher, hence unlikely to cause other
> > issues.
>
> Ah, thanks for digging. I agree that this is a small isolated bug, so
> the prereq check I showed would be a good test for it.
>
> IMHO it's worth doing. It looks like 8.7 is the only affected openssh
> version, but it is likely to cause confusion right when we release.

It was discussed[1] before, and I think the plan was to at least
mention the brokenness with 8.7 in the release notes, but guess I
dropped the ball by not raising it earlier.

Carlo

[1] https://lore.kernel.org/git/CAPUEspgnRFNRoFuEvP1hpY3iKukk3OnF4zk85wkdkmiVuPuRTw@mail.gmail.com/

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Wed, Nov 10, 2021 at 01:15:52AM -0800, Carlo Arenas wrote:

> On Wed, Nov 10, 2021 at 12:22 AM Jeff King <peff@peff.net> wrote:
> >
> > On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:
> >
> > > On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:
> > >
> > > >   - we're not really testing the desired behavior, just looking for a
> > > >     known-problem. The segfault may get fixed but we'd still have other
> > > >     bugs.
> > >
> > > This openssh bug was fixed in 8.8 per the release notes; indeed the
> > > fix[1] (which was misapplied but fixed next commit) looks familiar and
> > > it is just a straight up crasher, hence unlikely to cause other
> > > issues.
> >
> > Ah, thanks for digging. I agree that this is a small isolated bug, so
> > the prereq check I showed would be a good test for it.
> >
> > IMHO it's worth doing. It looks like 8.7 is the only affected openssh
> > version, but it is likely to cause confusion right when we release.
> 
> It was discussed[1] before, and I think the plan was to at least
> mention the brokenness with 8.7 in the release notes, but guess I
> dropped the ball by not raising it earlier.

Ah, thanks. I looked in the archive, but assumed any mention would have
been more recent (not realizing that Debian's packaging was simply
lagging the upstream releases by quite a bit).

-Peff

[PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7

[Carlo Marcelo Arenas Belón]

As discussed[1] earlier, make sure there are no surprises when ssh-keygen
crashes on some users of OpenSSH 8.7 that are trying ssh signing.

[1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Documentation/RelNotes/2.34.0.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt
index effab2ea4b..54dcc7240d 100644
--- a/Documentation/RelNotes/2.34.0.txt
+++ b/Documentation/RelNotes/2.34.0.txt
@@ -8,6 +8,9 @@ Backward compatibility notes
 
  * The "--preserve-merges" option of "git rebase" has been removed.
 
+ * The upcoming ssh signing feature is broken if used together with
+   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8
+   (or stay at 8.6)
 
 UI, Workflows & Features
 
-- 
2.34.0.rc1.349.g8f33748433

Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> As discussed[1] earlier, make sure there are no surprises when ssh-keygen
> crashes on some users of OpenSSH 8.7 that are trying ssh signing.
>
> [1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  Documentation/RelNotes/2.34.0.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt
> index effab2ea4b..54dcc7240d 100644
> --- a/Documentation/RelNotes/2.34.0.txt
> +++ b/Documentation/RelNotes/2.34.0.txt
> @@ -8,6 +8,9 @@ Backward compatibility notes
>  
>   * The "--preserve-merges" option of "git rebase" has been removed.
>  
> + * The upcoming ssh signing feature is broken if used together with
> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8
> +   (or stay at 8.6)

That may be correct, but it is NOT a backward compatibility note.

Re: [ANNOUNCE] Git v2.34.0-rc2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 10, 2021 at 07:35:47AM +0100, Johannes Altmanninger wrote:
>
>> > Fixes since v2.33
>> > -----------------
>> > 
>> >  * Doc update plus improved error reporting.
>> 
>> This should be something like
>> 
>>  * Warn when iconv(3) fails to reencode log messages.
>
> Yes, though note that we ended up reverting the warning. So probably it
> could be removed from the release notes entirely.

True.  Thanks for carefully reading these entries, all.

Re: [ANNOUNCE] Git v2.34.0-rc2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So it would be nice to have a more exact test, but without understanding
> the openssh bug, I think this is the best we can do in the meantime.

Sounds good.  We can also ensure that the key we are going to
generate here is actually usable before we leave the lazy-prereq
block, but we can leave it for another day.

Will apply.  Thanks.


>
> -- >8 --
> Subject: [PATCH] t/lib-gpg: avoid broken versions of ssh-keygen
>
> The "-Y find-principals" option of ssh-keygen seems to be broken in
> Debian's openssh-client 1:8.7p1-1, whereas it works fine in 1:8.4p1-5.
> This causes several failures for GPGSSH tests. We fulfill the
> prerequisite because generating the keys works fine, but actually
> verifying a signature causes results ranging from bogus results to
> ssh-keygen segfaulting.
>
> We can find the broken version during the prereq check by feeding it
> empty input. This should result in it complaining to stderr, but in the
> broken version it triggers the segfault, causing the GPGSSH tests to be
> skipped.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/lib-gpg.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index 1d8e5b5b7e..a3f285f515 100644
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -104,6 +104,12 @@ test_lazy_prereq GPGSSH '
>  	test $? != 127 || exit 1
>  	echo $ssh_version | grep -q "find-principals:missing signature file"
>  	test $? = 0 || exit 1;
> +
> +	# some broken versions of ssh-keygen segfault on find-principals;
> +	# avoid testing with them.
> +	ssh-keygen -Y find-principals -f /dev/null -s /dev/null
> +	test $? = 139 && exit 1
> +
>  	mkdir -p "${GNUPGHOME}" &&
>  	chmod 0700 "${GNUPGHOME}" &&
>  	(setfacl -k "${GNUPGHOME}" 2>/dev/null || true) &&

Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
>> As discussed[1] earlier, make sure there are no surprises when ssh-keygen
>> crashes on some users of OpenSSH 8.7 that are trying ssh signing.
>>
>> [1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/
>>
>> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>> ---
>>  Documentation/RelNotes/2.34.0.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt
>> index effab2ea4b..54dcc7240d 100644
>> --- a/Documentation/RelNotes/2.34.0.txt
>> +++ b/Documentation/RelNotes/2.34.0.txt
>> @@ -8,6 +8,9 @@ Backward compatibility notes
>>  
>>   * The "--preserve-merges" option of "git rebase" has been removed.
>>  
>> + * The upcoming ssh signing feature is broken if used together with
>> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8
>> +   (or stay at 8.6)
>
> That may be correct, but it is NOT a backward compatibility note.

So, here is what I plan to do.

diff --git c/Documentation/RelNotes/2.34.0.txt w/Documentation/RelNotes/2.34.0.txt
index effab2ea4b..6ed8b92e10 100644
--- c/Documentation/RelNotes/2.34.0.txt
+++ w/Documentation/RelNotes/2.34.0.txt
@@ -77,7 +77,10 @@ UI, Workflows & Features
  * "git fsck" has been taught to report mismatch between expected and
    actual types of an object better.
 
- * Use ssh public crypto for object and push-cert signing.
+ * In addition to GnuPG, ssh public crypto can be used for object and
+   push-cert signing.  Note that this feature cannot be used with
+   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid
+   using it unless you update to OpenSSH 8.8.
 
  * "git log --grep=string --author=name" learns to highlight hits just
    like "git grep string" does.

Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7

[Jeff King]

On Wed, Nov 10, 2021 at 02:11:29PM -0800, Junio C Hamano wrote:

> >> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt
> >> index effab2ea4b..54dcc7240d 100644
> >> --- a/Documentation/RelNotes/2.34.0.txt
> >> +++ b/Documentation/RelNotes/2.34.0.txt
> >> @@ -8,6 +8,9 @@ Backward compatibility notes
> >>  
> >>   * The "--preserve-merges" option of "git rebase" has been removed.
> >>  
> >> + * The upcoming ssh signing feature is broken if used together with
> >> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8
> >> +   (or stay at 8.6)
> >
> > That may be correct, but it is NOT a backward compatibility note.
> 
> So, here is what I plan to do.
> 
> diff --git c/Documentation/RelNotes/2.34.0.txt w/Documentation/RelNotes/2.34.0.txt
> index effab2ea4b..6ed8b92e10 100644
> --- c/Documentation/RelNotes/2.34.0.txt
> +++ w/Documentation/RelNotes/2.34.0.txt
> @@ -77,7 +77,10 @@ UI, Workflows & Features
>   * "git fsck" has been taught to report mismatch between expected and
>     actual types of an object better.
>  
> - * Use ssh public crypto for object and push-cert signing.
> + * In addition to GnuPG, ssh public crypto can be used for object and
> +   push-cert signing.  Note that this feature cannot be used with
> +   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid
> +   using it unless you update to OpenSSH 8.8.

Attaching to the existing gpg-ssh release note like this makes perfect
sense to me. Thanks for tying this one up.

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:

>  * The revision traversal API has been optimized by taking advantage
>    of the commit-graph, when available, to determine if a commit is
>    reachable from any of the existing refs.

I was thinking a bit about this change, specifically f45022dc2f
(connected: do not sort input revisions, 2021-08-09). At the time, Junio
said[1]:

  Sorting of positive side is done to help both performance and
  correctness in regular use of the traversal machinery, especially
  when reachability bitmap is not in effect, but on the negative side
  I do not think there is any downside to omit sorting offhand.  The
  only case that may get affected is when the revision.c::SLOP kicks
  in to deal with oddball commits with incorrect committer timestamps,
  but then the result of the sorting isn't to be trusted anyway, so...

But wouldn't failure to sort the commits actually _create_ such an
"oddball commits" scenario, because we'd see the UNINTERESTING commits
in an arbitrary order, especially with respect to the interesting ones?

I wondered if there were cases where this would change the output of the
command, and came up with this:

-- >8 --
git init -q repo
cd repo

# make a commit whose timestamp is offset from an arbitrary start point
# by $1 days. That gives us a deterministic commit ordering with no
# chance of any time-based slop.
commit_at() {
	base=1234567890
	unit=86400
	timestamp="@$((base + $1 * unit)) +0000" \
	GIT_COMMITTER_DATE=$timestamp \
	GIT_AUTHOR_DATE=$timestamp \
	git commit --allow-empty -qm "commit at day $1"
	git tag day-$1
}
commit_at 1
commit_at 2
commit_at 3
commit_at 4
commit_at 5
commit_at 6
commit_at 7
commit_at 8
commit_at 9
commit_at 10

# We ask to see day-1 and day-10, but the limit_list() traversal
# should drop day-1 as an ancestor of the others. There are two
# important things about the negative revisions:
#
#  - there are more than 5 of them, which will run out the SLOP counter
#
#  - we don't include ^day-2, as that could cause us to directly mark
#    its parent day-1 as UNINTERESTING
#
# Obviously this command line is contrived, but you could get the same
# inputs from a "rev-list $new_stuff --not --all" connectivity check,
# depending on where refs were pointing and what names they had.
tips='day-1 day-10 ^day-9 ^day-8 ^day-7 ^day-6 ^day-5 ^day-4'

# Now we can compare the sorted/unsorted inputs. I used "log" here just
# to make the output easier to understand.
git log --oneline $tips >expect
git log --oneline --unsorted-input $tips >actual
diff -u expect actual
-- >8 --

And the output I get is:

  --- expect	2021-11-11 06:50:22.148050216 -0500
  +++ actual	2021-11-11 06:50:22.152050263 -0500
  @@ -1 +1,2 @@
  +027d297 commit at day 1
   1dc264f commit at day 10

So indeed, we get confused by the unsorted input and give the wrong
answer. We should look at and propagate UNINTERESTING marks from day-9,
etc on down to day-1, but we don't.

Now in this case, we're sending too much output, which is OK for the
purposes of the connectivity check. It will just walk day-1 and its
tree unnecessarily, which is a performance loss but not incorrect.

My intuition is that this is _probably_ the only type of incorrect
answer we'd give (i.e., that we'd never go the other way, omitting an
object we should have included) because nobody would ever set the
UNINTERESTING flag unnecessarily, and everybody_uninteresting() does do
a full scan looking for any positive tips.

Still, I didn't see this subtlety mentioned in the earlier discussion,
and it's a bit alarming not to understand all of the possible
implications. I'm not sure if it rises to the level of something we want
to consider more or address before the release. Sorry to only come up
with this at the -rc2 stage, but I figure now is better than right after
the release. ;)

-Peff

[1] https://lore.kernel.org/git/xmqqa6lzwu31.fsf@gitster.g/

Re: [ANNOUNCE] Git v2.34.0-rc2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:
>
>>  * The revision traversal API has been optimized by taking advantage
>>    of the commit-graph, when available, to determine if a commit is
>>    reachable from any of the existing refs.
>
> I was thinking a bit about this change, specifically f45022dc2f
> (connected: do not sort input revisions, 2021-08-09). At the time, Junio
> said[1]:
>
>   Sorting of positive side is done to help both performance and
>   correctness in regular use of the traversal machinery, especially
>   when reachability bitmap is not in effect, but on the negative side
>   I do not think there is any downside to omit sorting offhand.  The
>   only case that may get affected is when the revision.c::SLOP kicks
>   in to deal with oddball commits with incorrect committer timestamps,
>   but then the result of the sorting isn't to be trusted anyway, so...
>
> But wouldn't failure to sort the commits actually _create_ such an
> "oddball commits" scenario, because we'd see the UNINTERESTING commits
> in an arbitrary order, especially with respect to the interesting ones?

You're right.

> So indeed, we get confused by the unsorted input and give the wrong
> answer. We should look at and propagate UNINTERESTING marks from day-9,
> etc on down to day-1, but we don't.
>
> Now in this case, we're sending too much output, which is OK for the
> purposes of the connectivity check. It will just walk day-1 and its
> tree unnecessarily, which is a performance loss but not incorrect.

The primary use of the traversal in check_connected() is when we
have new refs we haven't seen, and they go to the positive end of
the traversal, which will end in the refs we do have (there may be
tons).  The idea is that when one or more of the new refs are truly
"new" in the sense that one or more the objects necessary to connect
them to our refs do not exist, not even in the "not reachable but
not yet pruned" state, this traversal will hit a missing object and
will error out.  So, it is alarming that "day-1" is shown without
painted uninteresting via any of the negative [day-4..day-9]
commits.  Which means, if we are checking if we need to initiate a
real fetch to connect day-1 and day-10 to our DAG, when we think we
have [day-4..day-9] and everything behind them, we stopped traversal
before seeing all the "objects necessary to connect them to our
refs".  If day-2 were missing in our repository, we would have
noticed if we did traversal from sorted tips, but the unsorted
traversal happily misses it.

Not that noticing that day-2 is missing from our repository does not
help much in *this* particular case, though.  It is likely that a
common negotiation would say "I have [day-4..day-9], and want day-1
and day-10", that is reponded with "OK, I know enough, here is
day-10 and its tree/blob that would be missing from a healthy clone
with everything behind day-9", and it won't include day-2 (nor
day-1).  So in this particular example, it would not matter if the
new unsorted traversal is subtly broken (I think the extent of the
damage is similar to making the SLOP problem deliberately worse),
but I am not sure if there are other failure modes that would yield
outright incorrect result.

> My intuition is that this is _probably_ the only type of incorrect
> answer we'd give (i.e., that we'd never go the other way, omitting an
> object we should have included) because nobody would ever set the
> UNINTERESTING flag unnecessarily, and everybody_uninteresting() does do
> a full scan looking for any positive tips.
>
> Still, I didn't see this subtlety mentioned in the earlier discussion,
> and it's a bit alarming not to understand all of the possible
> implications. I'm not sure if it rises to the level of something we want
> to consider more or address before the release. Sorry to only come up
> with this at the -rc2 stage, but I figure now is better than right after
> the release. ;)
>
> -Peff
>
> [1] https://lore.kernel.org/git/xmqqa6lzwu31.fsf@gitster.g/

We probably should revert this step as it can affect correctness in
a big way, but I wonder if the other steps in the same series, or
other topic that came later, rely on it.

At the very least, I think this may be prudent during -rc period,
but on the other hand, I do not know offhand what would later
pursuade us to reinstate it and convince us that it is a safe thing
to do.

 connected.c | 1 -
 1 file changed, 1 deletion(-)

diff --git c/connected.c w/connected.c
index cf68e37a97..35bd4a2638 100644
--- c/connected.c
+++ w/connected.c
@@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	if (opt->progress)
 		strvec_pushf(&rev_list.args, "--progress=%s",
 			     _("Checking connectivity"));
-	strvec_push(&rev_list.args, "--unsorted-input");
 
 	rev_list.git_cmd = 1;
 	rev_list.env = opt->env;

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:

> > Now in this case, we're sending too much output, which is OK for the
> > purposes of the connectivity check. It will just walk day-1 and its
> > tree unnecessarily, which is a performance loss but not incorrect.
> 
> The primary use of the traversal in check_connected() is when we
> have new refs we haven't seen, and they go to the positive end of
> the traversal, which will end in the refs we do have (there may be
> tons).  The idea is that when one or more of the new refs are truly
> "new" in the sense that one or more the objects necessary to connect
> them to our refs do not exist, not even in the "not reachable but
> not yet pruned" state, this traversal will hit a missing object and
> will error out.  So, it is alarming that "day-1" is shown without
> painted uninteresting via any of the negative [day-4..day-9]
> commits.  Which means, if we are checking if we need to initiate a
> real fetch to connect day-1 and day-10 to our DAG, when we think we
> have [day-4..day-9] and everything behind them, we stopped traversal
> before seeing all the "objects necessary to connect them to our
> refs".  If day-2 were missing in our repository, we would have
> noticed if we did traversal from sorted tips, but the unsorted
> traversal happily misses it.

Yes, but if day-2 were missing in our repository, then we are already
corrupt. And in most cases we would not notice adding a new ref that is
also corrupt (e.g., imagine adding _just_ day-10 which is a descendent
of day-2, but we stop traversal at day-9 when we see that we already
have it reachable).

So I don't think it is actually changing the check_connected() outcome.
I couldn't come up with a case where we should be checking a commit and
don't. Only the other way around.

> Not that noticing that day-2 is missing from our repository does not
> help much in *this* particular case, though.  It is likely that a
> common negotiation would say "I have [day-4..day-9], and want day-1
> and day-10", that is reponded with "OK, I know enough, here is
> day-10 and its tree/blob that would be missing from a healthy clone
> with everything behind day-9", and it won't include day-2 (nor
> day-1).  So in this particular example, it would not matter if the
> new unsorted traversal is subtly broken (I think the extent of the
> damage is similar to making the SLOP problem deliberately worse),
> but I am not sure if there are other failure modes that would yield
> outright incorrect result.

Yes, I think that framing is right: it is making SLOP much worse. We
could similarly have had bogus timestamps in those commits which would
cause the same outcome. So in that sense it is nothing new. On the other
hand, I wonder how often it will cause extra traversal work (keeping in
mind that this commit traversal is just the first stage; after we find
the commits, then we talk all of their trees, which is the more
expensive part).

For the case of adding new commits directly on top of another branch, I
think there would be no change. But any time you have to walk down to a
common fork point (e.g., imagine I made a new branch forked from an old
bit of history), we may fail to find that. I haven't quite constructed
an example, but I have a feeling we could end up walking over
arbitrarily long segments of history.

> We probably should revert this step as it can affect correctness in
> a big way, but I wonder if the other steps in the same series, or
> other topic that came later, rely on it.

I looked them over, and I think this is pretty independent (with the
exception of the refactoring of the no_walk/unsorted flags, but
obviously that had to come first).

> At the very least, I think this may be prudent during -rc period,
> but on the other hand, I do not know offhand what would later
> pursuade us to reinstate it and convince us that it is a safe thing
> to do.
> [...]
> diff --git c/connected.c w/connected.c
> index cf68e37a97..35bd4a2638 100644
> --- c/connected.c
> +++ w/connected.c
> @@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  	if (opt->progress)
>  		strvec_pushf(&rev_list.args, "--progress=%s",
>  			     _("Checking connectivity"));
> -	strvec_push(&rev_list.args, "--unsorted-input");
>  
>  	rev_list.git_cmd = 1;
>  	rev_list.env = opt->env;

This seems like a pretty safe and minimal backing-out for the -rc
period. We would still ship with "--unsorted-input" as an option (and
mentioned in the docs), though. If we're worried that it might be a dead
end and we don't want to support it, we could revert f45022dc2f
(connected: do not sort input revisions, 2021-08-09) entirely. That
carries a little more risk of accidentally breaking something during the
revert, but from my reading of the patch it should be pretty safe.

I'd be curious to hear Patrick's thoughts on the whole thing.

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:
>
>> ...  So in this particular example, it would not matter if the
>> new unsorted traversal is subtly broken (I think the extent of the
>> damage is similar to making the SLOP problem deliberately worse),
>> but I am not sure if there are other failure modes that would yield
>> outright incorrect result.
>
> Yes, I think that framing is right: it is making SLOP much worse. We
> could similarly have had bogus timestamps in those commits which would
> cause the same outcome. So in that sense it is nothing new. On the other
> hand, I wonder how often it will cause extra traversal work (keeping in
> mind that this commit traversal is just the first stage; after we find
> the commits, then we talk all of their trees, which is the more
> expensive part).
>
> For the case of adding new commits directly on top of another branch, I
> think there would be no change. But any time you have to walk down to a
> common fork point (e.g., imagine I made a new branch forked from an old
> bit of history), we may fail to find that. I haven't quite constructed
> an example, but I have a feeling we could end up walking over
> arbitrarily long segments of history.
> ...
> I'd be curious to hear Patrick's thoughts on the whole thing.

Yes.  I'm tempted to wait for him to chime in.

Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2021-11-11 at 09:16:52, Jeff King wrote:
>> On Wed, Nov 10, 2021 at 02:11:29PM -0800, Junio C Hamano wrote:
>>  ....
>> > + * In addition to GnuPG, ssh public crypto can be used for object and
>> > +   push-cert signing.  Note that this feature cannot be used with
>> > +   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid
>> > +   using it unless you update to OpenSSH 8.8.
>> 
>> Attaching to the existing gpg-ssh release note like this makes perfect
>> sense to me. Thanks for tying this one up.
>
> Since this now affects running the testsuite on Debian unstable, we
> probably need to also fix the testsuite such that the GPGSSH
> prerequisite fails if we're using OpenSSH 8.7 so that developers and
> distros aren't negatively affected.

Yes, I think YYtgD8VT/0vuIHRX@coredump.intra.peff.net is a good
thing to have, which is ca7a5bf4 (t/lib-gpg: avoid broken versions
of ssh-keygen, 2021-11-10) that was merged a few days ago.

Thanks for being extra careful.

Re: [ANNOUNCE] Git v2.34.0-rc2

[Patrick Steinhardt]

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

On Thu, Nov 11, 2021 at 03:23:08PM -0500, Jeff King wrote:
> On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:
> 
> > > Now in this case, we're sending too much output, which is OK for the
> > > purposes of the connectivity check. It will just walk day-1 and its
> > > tree unnecessarily, which is a performance loss but not incorrect.
> > 
> > The primary use of the traversal in check_connected() is when we
> > have new refs we haven't seen, and they go to the positive end of
> > the traversal, which will end in the refs we do have (there may be
> > tons).  The idea is that when one or more of the new refs are truly
> > "new" in the sense that one or more the objects necessary to connect
> > them to our refs do not exist, not even in the "not reachable but
> > not yet pruned" state, this traversal will hit a missing object and
> > will error out.  So, it is alarming that "day-1" is shown without
> > painted uninteresting via any of the negative [day-4..day-9]
> > commits.  Which means, if we are checking if we need to initiate a
> > real fetch to connect day-1 and day-10 to our DAG, when we think we
> > have [day-4..day-9] and everything behind them, we stopped traversal
> > before seeing all the "objects necessary to connect them to our
> > refs".  If day-2 were missing in our repository, we would have
> > noticed if we did traversal from sorted tips, but the unsorted
> > traversal happily misses it.
> 
> Yes, but if day-2 were missing in our repository, then we are already
> corrupt. And in most cases we would not notice adding a new ref that is
> also corrupt (e.g., imagine adding _just_ day-10 which is a descendent
> of day-2, but we stop traversal at day-9 when we see that we already
> have it reachable).
> 
> So I don't think it is actually changing the check_connected() outcome.
> I couldn't come up with a case where we should be checking a commit and
> don't. Only the other way around.
> 
> > Not that noticing that day-2 is missing from our repository does not
> > help much in *this* particular case, though.  It is likely that a
> > common negotiation would say "I have [day-4..day-9], and want day-1
> > and day-10", that is reponded with "OK, I know enough, here is
> > day-10 and its tree/blob that would be missing from a healthy clone
> > with everything behind day-9", and it won't include day-2 (nor
> > day-1).  So in this particular example, it would not matter if the
> > new unsorted traversal is subtly broken (I think the extent of the
> > damage is similar to making the SLOP problem deliberately worse),
> > but I am not sure if there are other failure modes that would yield
> > outright incorrect result.
> 
> Yes, I think that framing is right: it is making SLOP much worse. We
> could similarly have had bogus timestamps in those commits which would
> cause the same outcome. So in that sense it is nothing new. On the other
> hand, I wonder how often it will cause extra traversal work (keeping in
> mind that this commit traversal is just the first stage; after we find
> the commits, then we talk all of their trees, which is the more
> expensive part).
> 
> For the case of adding new commits directly on top of another branch, I
> think there would be no change. But any time you have to walk down to a
> common fork point (e.g., imagine I made a new branch forked from an old
> bit of history), we may fail to find that. I haven't quite constructed
> an example, but I have a feeling we could end up walking over
> arbitrarily long segments of history.

Sorry, but I'm currently completely loaded with work and thus didn't
find the capacity to have a deeper look yet and will probably not find
the time for a few more days. So the earliest I can have a look at this
is probably beginning next week.

With that in mind, I'm happy to have this change reverted for now, as it
is...

> > We probably should revert this step as it can affect correctness in
> > a big way, but I wonder if the other steps in the same series, or
> > other topic that came later, rely on it.
> 
> I looked them over, and I think this is pretty independent (with the
> exception of the refactoring of the no_walk/unsorted flags, but
> obviously that had to come first).

.. completely independent of the other patches in this series and can be
reverted on its own. Only question is whether we also want to revert the
patch introducing this option in the first place given that it would end
up without a user afterwards.

Patrick

> > At the very least, I think this may be prudent during -rc period,
> > but on the other hand, I do not know offhand what would later
> > pursuade us to reinstate it and convince us that it is a safe thing
> > to do.
> > [...]
> > diff --git c/connected.c w/connected.c
> > index cf68e37a97..35bd4a2638 100644
> > --- c/connected.c
> > +++ w/connected.c
> > @@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> >  	if (opt->progress)
> >  		strvec_pushf(&rev_list.args, "--progress=%s",
> >  			     _("Checking connectivity"));
> > -	strvec_push(&rev_list.args, "--unsorted-input");
> >  
> >  	rev_list.git_cmd = 1;
> >  	rev_list.env = opt->env;
> 
> This seems like a pretty safe and minimal backing-out for the -rc
> period. We would still ship with "--unsorted-input" as an option (and
> mentioned in the docs), though. If we're worried that it might be a dead
> end and we don't want to support it, we could revert f45022dc2f
> (connected: do not sort input revisions, 2021-08-09) entirely. That
> carries a little more risk of accidentally breaking something during the
> revert, but from my reading of the patch it should be pretty safe.
> 
> I'd be curious to hear Patrick's thoughts on the whole thing.
> 
> -Peff

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Mon, Nov 15, 2021 at 04:06:45PM +0100, Patrick Steinhardt wrote:

> Sorry, but I'm currently completely loaded with work and thus didn't
> find the capacity to have a deeper look yet and will probably not find
> the time for a few more days. So the earliest I can have a look at this
> is probably beginning next week.
> 
> With that in mind, I'm happy to have this change reverted for now, as it
> is...

Thanks for chiming in (and again, sorry for bringing this up so late in
the cycle).

> > > We probably should revert this step as it can affect correctness in
> > > a big way, but I wonder if the other steps in the same series, or
> > > other topic that came later, rely on it.
> > 
> > I looked them over, and I think this is pretty independent (with the
> > exception of the refactoring of the no_walk/unsorted flags, but
> > obviously that had to come first).
> 
> .. completely independent of the other patches in this series and can be
> reverted on its own. Only question is whether we also want to revert the
> patch introducing this option in the first place given that it would end
> up without a user afterwards.

It looks like Junio queued a revert of the whole patch in a7df4f52af
(Revert "connected: do not sort input revisions", 2021-11-11), which is
on "master". So I think we should have a clean slate to look at this in
the next cycle.

-Peff

Re: [ANNOUNCE] Git v2.34.0-rc2

[Jeff King]

On Thu, Nov 11, 2021 at 03:23:09PM -0500, Jeff King wrote:

> Yes, I think that framing is right: it is making SLOP much worse. We
> could similarly have had bogus timestamps in those commits which would
> cause the same outcome. So in that sense it is nothing new. On the other
> hand, I wonder how often it will cause extra traversal work (keeping in
> mind that this commit traversal is just the first stage; after we find
> the commits, then we talk all of their trees, which is the more
> expensive part).
> 
> For the case of adding new commits directly on top of another branch, I
> think there would be no change. But any time you have to walk down to a
> common fork point (e.g., imagine I made a new branch forked from an old
> bit of history), we may fail to find that. I haven't quite constructed
> an example, but I have a feeling we could end up walking over
> arbitrarily long segments of history.

I was playing around with this a bit more, and there is one subtlety in
the "day-10" snippet I showed that I hadn't noted before. It's important
the day-1 does not have any parents. If we used day-2 instead, then
limit_list() would insert its parent (day-1, in this case) into the
queue, without an UNINTERESTING flag (because it's the parent of
something interesting).  And thus when we call still_interesting(), we
would never decrement the slop counter, because we know we are still
walking back to something potentially interesting.

This "works" because we put the new commit at the end of the list via
commit_list_insert_by_date(). That can be fooled, of course, because
it's assuming the list is already in sorted order (which it isn't). So
there could be an "old" commit at the front, and we place the parent in
front of that, even though it's UNINTERESTING descendants are further
back in the list.

So I do think we could walk an arbitrary string of history in this way,
all the way down to the root, or to something else pointed to by a ref
tip. Here's the example I came up with:

-- >8 --
git init -q repo
cd repo

commit_at() {
	echo $1 >$1
	git add .
	base=1234567890
	unit=86400
	timestamp="@$((base + $1 * unit)) +0000"
	GIT_COMMITTER_DATE=$timestamp \
	GIT_AUTHOR_DATE=$timestamp \
	git commit -qm "commit at day $1"
}

# imagine a bunch of base history
for i in $(seq 100); do
	commit_at $i
done
git tag base

# And then we have some older branches hanging around.
for i in $(seq 1 10); do
	git checkout -b branch-$i base~$((60+$i))
done

# But also a newer one; it's important that this refname
# sort after the other ones, because that's what confuses
# the sorting.
git branch new-branch

# and then somebody pushes/fetches a branch based on an old part of history,
# newer than our old branches, but older than our new one.
#
# We won't actually create the branch here, because we're simulating the state
# before the ref is created, when we do the connectivity check.
old_commit=$(git rev-parse base~50)
new_commit=$(echo foo | git commit-tree -p $old_commit HEAD^{tree})

# and now here's the connectivity check we would do
git rev-list $new_commit --not --all >expect
git rev-list --unsorted-input $new_commit --not --all >actual
diff -u expect actual
-- >8 --

That will report all of base~60..base~50 in the output, when it should
just report the single new commit.

I don't think any of this changes the plan for the 2.34 release (in
fact, it makes me more confident that reverting this change was the
right thing to do). I'm just recording my notes here for revisiting the
topic later.

My suspicion is that there's no easy way to make this work. We're
violating the assumption in still_interesting() that it can easily find
the lowest-date commit. We could drop that assumption for the unsorted
case, but then I think we'd be forced to walk all the way to the root
commits, which is even worse than sorting the tips.

I suspect a better solution would be to make use of generation numbers
from the commit graph if we have them. The --topo-order stuff already
does this, and I kind of wonder if we could piggy-back on that.

-Peff