{"thread":{"id":"56863","subject":"[ANNOUNCE] Git v2.34.0-rc2","startedAt":"2021-11-10T00:59:34Z","lastAt":"2021-11-15T16:52:58Z","messageCount":23,"participants":["Junio C Hamano","Jeff King","Johannes Altmanninger","Carlo Arenas","Carlo Marcelo Arenas Belón","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"440791","messageId":"xmqq4k8kzuz2.fsf@gitster.g","threadId":"56863","inReplyTo":null,"subject":"[ANNOUNCE] Git v2.34.0-rc2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T00:59:29Z","receivedAt":"2021-11-10T00:59:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A release candidate Git v2.34.0-rc2 is now available for testing at\nthe usual places.  It is comprised of 786 non-merge commits since\nv2.33.0, contributed by 93 people, 28 of which are new faces [*].\n\nThe tarballs are found at:\n\n    https://www.kernel.org/pub/software/scm/git/testing/\n\nThe following public repositories all have a copy of the\n'v2.34.0-rc2' tag and the 'master' branch that the tag points at:\n\n  url = https://git.kernel.org/pub/scm/git/git\n  url = https://kernel.googlesource.com/pub/scm/git/git\n  url = git://repo.or.cz/alt-git.git\n  url = https://github.com/gitster/git\n\nNew contributors whose contributions weren't in v2.33.0 are as follows.\nWelcome to the Git development community!\n\n  Alan Blotz, Azeem Bande-Ali, Dr. Matthias St. Pierre, Eli\n  Schwartz, git.mexon@spamgourmet.com, Glen Choo, Hamza Mahfooz,\n  Joel Klinghed, Johannes Altmanninger, Jonas Kittner, Kim\n  Altintop, Kyle Zhao, Mahi Kolla, Marvin Häuser, Mickey Endito,\n  Rob Browning, Robert Estelle, Robert Leftwich, Robin Dupret,\n  Tal Kelrich, Tassilo Horn, Thomas De Zeeuw, USAMI Kenta, Victor\n  Gambier, Victoria Dye, Wesley Schwengle, Xingman Chen, and Zoker.\n\nReturning contributors who helped this release are as follows.\nThanks for your continued support.\n\n  Adam Dinwoodie, Ævar Arnfjörð Bjarmason, Alban Gruin, Alex\n  Henrie, Alex Riesen, Andrzej Hunt, Atharva Raykar, Bagas Sanjaya,\n  Ben Boeckel, brian m. carlson, Carlo Arenas, Carlo Marcelo\n  Arenas Belón, Christian Couder, David Aguilar, David Turner,\n  Derrick Stolee, Đoàn Trần Công Danh, Elijah Newren, Emily\n  Shaffer, Emir Sarı, Eric Sunshine, Eric Wong, Fabian Stelzer,\n  Felipe Contreras, Greg Hurrell, Han-Wen Nienhuys, Hariom Verma,\n  Jacob Keller, Jacob Vosmaer, Jean-Noël Avila, Jeff Hostetler,\n  Jeff King, Jiang Xin, Johannes Schindelin, Johannes Sixt,\n  Jonathan Nieder, Jonathan Tan, Josh Steadmon, Junio C Hamano,\n  Kaartic Sivaraam, Lénaïc Huard, Martin Ågren, Matheus Tavares,\n  Matheus Tavares Bernardino, Matthias Aßhauer, Mike Hommey,\n  Miriam Rubio, Orgad Shaneh, Patrick Steinhardt, Philip Oakley,\n  Philippe Blain, Phillip Wood, Pranit Bauva, Randall S. Becker,\n  René Scharfe, Sergey Organov, Shourya Shukla, SZEDER Gábor,\n  Takashi Iwai, Tanushree Tumane, Taylor Blau, Teng Long, Todd\n  Zullinger, Ulrich Windl, and ZheNing Hu.\n\n[*] We are counting not just the authorship contribution but issue\n    reporting, mentoring, helping and reviewing that are recorded in\n    the commit trailers.\n\n----------------------------------------------------------------\n\nGit 2.34 Release Notes (draft)\n==============================\n\nUpdates since Git 2.33\n----------------------\n\nBackward compatibility notes\n\n * The \"--preserve-merges\" option of \"git rebase\" has been removed.\n\n\nUI, Workflows & Features\n\n * Pathname expansion (like \"~username/\") learned a way to specify a\n   location relative to Git installation (e.g. its $sharedir which is\n   $(prefix)/share), with \"%(prefix)\".\n\n * Use `ort` instead of `recursive` as the default merge strategy.\n\n * The userdiff pattern for \"java\" language has been updated.\n\n * \"git rebase\" by default skips changes that are equivalent to\n   commits that are already in the history the branch is rebased onto;\n   give messages when this happens to let the users be aware of\n   skipped commits, and also teach them how to tell \"rebase\" to keep\n   duplicated changes.\n\n * The advice message that \"git cherry-pick\" gives when it asks\n   conflicted replay of a commit to be resolved by the end user has\n   been updated.\n\n * After \"git clone --recurse-submodules\", all submodules are cloned\n   but they are not by default recursed into by other commands.  With\n   submodule.stickyRecursiveClone configuration set, submodule.recurse\n   configuration is set to true in a repository created by \"clone\"\n   with \"--recurse-submodules\" option.\n\n * The logic for auto-correction of misspelt subcommands learned to go\n   interactive when the help.autocorrect configuration variable is set\n   to 'prompt'.\n\n * \"git maintenance\" scheduler learned to use systemd timers as a\n   possible backend.\n\n * \"git diff --submodule=diff\" showed failure from run_command() when\n   trying to run diff inside a submodule, when the user manually\n   removes the submodule directory.\n\n * \"git bundle unbundle\" learned to show progress display.\n\n * In cone mode, the sparse-index code path learned to remove ignored\n   files (like build artifacts) outside the sparse cone, allowing the\n   entire directory outside the sparse cone to be removed, which is\n   especially useful when the sparse patterns change.\n\n * Taking advantage of the CGI interface, http-backend has been\n   updated to enable protocol v2 automatically when the other side\n   asks for it.\n\n * The credential-cache helper has been adjusted to Windows.\n\n * The error in \"git help no-such-git-command\" is handled better.\n\n * The unicode character width table (used for output alignment) has\n   been updated.\n\n * The ref iteration code used to optionally allow dangling refs to be\n   shown, which has been tightened up.\n\n * \"git add\", \"git mv\", and \"git rm\" have been adjusted to avoid\n   updating paths outside of the sparse-checkout definition unless\n   the user specifies a \"--sparse\" option.\n\n * \"git repack\" has been taught to generate multi-pack reachability\n   bitmaps.\n\n * \"git fsck\" has been taught to report mismatch between expected and\n   actual types of an object better.\n\n * Use ssh public crypto for object and push-cert signing.\n\n * \"git log --grep=string --author=name\" learns to highlight hits just\n   like \"git grep string\" does.\n\n\n\nPerformance, Internal Implementation, Development Support etc.\n\n * \"git bisect\" spawned \"git show-branch\" only to pretty-print the\n   title of the commit after checking out the next version to be\n   tested; this has been rewritten in C.\n\n * \"git add\" can work better with the sparse index.\n\n * Support for ancient versions of cURL library (pre 7.19.4) has been\n   dropped.\n\n * A handful of tests that assumed implementation details of files\n   backend for refs have been cleaned up.\n\n * trace2 logs learned to show parent process name to see in what\n   context Git was invoked.\n\n * Loading of ref tips to prepare for common ancestry negotiation in\n   \"git fetch-pack\" has been optimized by taking advantage of the\n   commit graph when available.\n\n * Remind developers that the userdiff patterns should be kept simple\n   and permissive, assuming that the contents they apply are always\n   syntactically correct.\n\n * The current implementation of GIT_TEST_FAIL_PREREQS is broken in\n   that checking for the lack of a prerequisite would not work.  Avoid\n   the use of \"if ! test_have_prereq X\" in a test script.\n\n * The revision traversal API has been optimized by taking advantage\n   of the commit-graph, when available, to determine if a commit is\n   reachable from any of the existing refs.\n\n * \"git fetch --quiet\" optimization to avoid useless computation of\n   info that will never be displayed.\n\n * Callers from older advice_config[] based API has been updated to\n   use the newer advice_if_enabled() and advice_enabled() API.\n\n * Teach \"test_pause\" and \"debug\" helpers to allow using the HOME and\n   TERM environment variables the user usually uses.\n\n * \"make INSTALL_STRIP=-s install\" allows the installation step to use\n   \"install -s\" to strip the binaries as they get installed.\n\n * Code that handles large number of refs in the \"git fetch\" code\n   path has been optimized.\n\n * The reachability bitmap file used to be generated only for a single\n   pack, but now we've learned to generate bitmaps for history that\n   span across multiple packfiles.\n\n * The code to make \"git grep\" recurse into submodules has been\n   updated to migrate away from the \"add submodule's object store as\n   an alternate object store\" mechanism (which is suboptimal).\n\n * The tracing of process ancestry information has been enhanced.\n\n * Reduce number of write(2) system calls while sending the\n   ref advertisement.\n\n * Update the build procedure to use the \"-pedantic\" build when\n   DEVELOPER makefile macro is in effect.\n\n * Large part of \"git submodule add\" gets rewritten in C.\n\n * The run-command API has been updated so that the callers can easily\n   ask the file descriptors open for packfiles to be closed immediately\n   before spawning commands that may trigger auto-gc.\n\n * An oddball OPTION_ARGUMENT feature has been removed from the\n   parse-options API.\n\n * The mergesort implementation used to sort linked list has been\n   optimized.\n\n * Remove external declaration of functions that no longer exist.\n\n * \"git multi-pack-index write --bitmap\" learns to propagate the\n   hashcache from original bitmap to resulting bitmap.\n\n * CI learns to run the leak sanitizer builds.\n\n * \"git grep --recurse-submodules\" takes trees and blobs from the\n   submodule repository, but the textconv settings when processing a\n   blob from the submodule is not taken from the submodule repository.\n   A test is added to demonstrate the issue, without fixing it.\n\n * Teach \"git help -c\" into helping the command line completion of\n   configuration variables.\n\n * When \"git cmd -h\" shows more than one line of usage text (e.g.\n   the cmd subcommand may take sub-sub-command), parse-options API\n   learned to align these lines, even across i18n/l10n.\n\n * Prevent \"make sparse\" from running for the source files that\n   haven't been modified.\n\n * The codepath to write a new version of .midx multi-pack index files\n   has learned to release the mmaped memory holding the current\n   version of .midx before removing them from the disk, as some\n   platforms do not allow removal of a file that still has mapping.\n\n * A new feature has been added to abort early in the test framework.\n\n\nFixes since v2.33\n-----------------\n\n * Input validation of \"git pack-objects --stdin-packs\" has been\n   corrected.\n\n * Bugfix for common ancestor negotiation recently introduced in \"git\n   push\" code path.\n\n * \"git pull\" had various corner cases that were not well thought out\n   around its --rebase backend, e.g. \"git pull --ff-only\" did not stop\n   but went ahead and rebased when the history on other side is not a\n   descendant of our history.  The series tries to fix them up.\n\n * \"git apply\" miscounted the bytes and failed to read to the end of\n   binary hunks.\n\n * \"git range-diff\" code clean-up.\n\n * \"git commit --fixup\" now works with \"--edit\" again, after it was\n   broken in v2.32.\n\n * Use upload-artifacts v1 (instead of v2) for 32-bit linux, as the\n   new version has a blocker bug for that architecture.\n\n * Checking out all the paths from HEAD during the last conflicted\n   step in \"git rebase\" and continuing would cause the step to be\n   skipped (which is expected), but leaves MERGE_MSG file behind in\n   $GIT_DIR and confuses the next \"git commit\", which has been\n   corrected.\n\n * Various bugs in \"git rebase -r\" have been fixed.\n\n * mmap() imitation used to call xmalloc() that dies upon malloc()\n   failure, which has been corrected to just return an error to the\n   caller to be handled.\n\n * \"git diff --relative\" segfaulted and/or produced incorrect result\n   when there are unmerged paths.\n\n * The delayed checkout code path in \"git checkout\" etc. were chatty\n   even when --quiet and/or --no-progress options were given.\n\n * \"git branch -D <branch>\" used to refuse to remove a broken branch\n   ref that points at a missing commit, which has been corrected.\n\n * Build update for Apple clang.\n\n * The parser for the \"--nl\" option of \"git column\" has been\n   corrected.\n\n * \"git upload-pack\" which runs on the other side of \"git fetch\"\n   forgot to take the ref namespaces into account when handling\n   want-ref requests.\n\n * The sparse-index support can corrupt the index structure by storing\n   a stale and/or uninitialized data, which has been corrected.\n\n * Buggy tests could damage repositories outside the throw-away test\n   area we created.  We now by default export GIT_CEILING_DIRECTORIES\n   to limit the damage from such a stray test.\n\n * Even when running \"git send-email\" without its own threaded\n   discussion support, a threading related header in one message is\n   carried over to the subsequent message to result in an unwanted\n   threading, which has been corrected.\n\n * The output from \"git fast-export\", when its anonymization feature\n   is in use, showed an annotated tag incorrectly.\n\n * Doc update plus improved error reporting.\n\n * Recent \"diff -m\" changes broke \"gitk\", which has been corrected.\n\n * Regression fix.\n\n * The \"git apply -3\" code path learned not to bother the lower level\n   merge machinery when the three-way merge can be trivially resolved\n   without the content level merge.  This fixes a regression caused by\n   recent \"-3way first and fall back to direct application\" change.\n\n * The code that optionally creates the *.rev reverse index file has\n   been optimized to avoid needless computation when it is not writing\n   the file out.\n\n * \"git range-diff -I... <range> <range>\" segfaulted, which has been\n   corrected.\n\n * The order in which various files that make up a single (conceptual)\n   packfile has been reevaluated and straightened up.  This matters in\n   correctness, as an incomplete set of files must not be shown to a\n   running Git.\n\n * The \"mode\" word is useless in a call to open(2) that does not\n   create a new file.  Such a call in the files backend of the ref\n   subsystem has been cleaned up.\n\n * \"git update-ref --stdin\" failed to flush its output as needed,\n   which potentially led the conversation to a deadlock.\n\n * When \"git am --abort\" fails to abort correctly, it still exited\n   with exit status of 0, which has been corrected.\n\n * Correct nr and alloc members of strvec struct to be of type size_t.\n\n * \"git stash\", where the tentative change involves changing a\n   directory to a file (or vice versa), was confused, which has been\n   corrected.\n\n * \"git clone\" from a repository whose HEAD is unborn into a bare\n   repository didn't follow the branch name the other side used, which\n   is corrected.\n\n * \"git cvsserver\" had a long-standing bug in its authentication code,\n   which has finally been corrected (it is unclear and is a separate\n   question if anybody is seriously using it, though).\n\n * \"git difftool --dir-diff\" mishandled symbolic links.\n\n * Sensitive data in the HTTP trace were supposed to be redacted, but\n   we failed to do so in HTTP/2 requests.\n\n * \"make clean\" has been updated to remove leftover .depend/\n   directories, even when it is not told to use them to compute header\n   dependencies.\n\n * Protocol v0 clients can get stuck parsing a malformed feature line.\n\n * A few kinds of changes \"git status\" can show were not documented.\n   (merge d2a534c515 ja/doc-status-types-and-copies later to maint).\n\n * The mergesort implementation used to sort linked list has been\n   optimized.\n   (merge c90cfc225b rs/mergesort later to maint).\n\n * An editor session launched during a Git operation (e.g. during 'git\n   commit') can leave the terminal in a funny state.  The code path\n   has updated to save the terminal state before, and restore it\n   after, it spawns an editor.\n   (merge 3d411afabc cm/save-restore-terminal later to maint).\n\n * \"git cat-file --batch\" with the \"--batch-all-objects\" option is\n   supposed to iterate over all the objects found in a repository, but\n   it used to translate these object names using the replace mechanism,\n   which defeats the point of enumerating all objects in the repository.\n   This has been corrected.\n   (merge bf972896d7 jk/cat-file-batch-all-wo-replace later to maint).\n\n * Recent sparse-index work broke safety against attempts to add paths\n   with trailing slashes to the index, which has been corrected.\n   (merge c8ad9d04c6 rs/make-verify-path-really-verify-again later to maint).\n\n * The \"--color-lines\" and \"--color-by-age\" options of \"git blame\"\n   have been missing, which are now documented.\n   (merge 8c32856133 bs/doc-blame-color-lines later to maint).\n\n * The PATH used in CI job may be too wide and let incompatible dlls\n   to be grabbed, which can cause the build&test to fail.  Tighten it.\n   (merge 7491ef6198 js/windows-ci-path-fix later to maint).\n\n * Avoid performance measurements from getting ruined by gc and other\n   housekeeping pauses interfering in the middle.\n   (merge be79131a53 rs/disable-gc-during-perf-tests later to maint).\n\n * Stop \"git add --dry-run\" from creating new blob and tree objects.\n   (merge e578d0311d rs/add-dry-run-without-objects later to maint).\n\n * \"git commit\" gave duplicated error message when the object store\n   was unwritable, which has been corrected.\n   (merge 4ef91a2d79 ab/fix-commit-error-message-upon-unwritable-object-store later to maint).\n\n * Recent sparse-index addition, namely any use of index_name_pos(),\n   can expand sparse index entries and breaks any code that walks\n   cache-tree or existing index entries.  One such instance of such a\n   breakage has been corrected.\n\n * The xxdiff difftool backend can exit with status 128, which the\n   difftool-helper that launches the backend takes as a significant\n   failure, when it is not significant at all.  Work it around.\n   (merge 571f4348dd da/mergetools-special-case-xxdiff-exit-128 later to maint).\n\n * Improve test framework around unwritable directories.\n   (merge 5d22e18965 ab/test-cleanly-recreate-trash-directory later to maint).\n\n * \"git push\" client talking to an HTTP server did not diagnose the\n   lack of the final status report from the other side correctly,\n   which has been corrected.\n   (merge c5c3486f38 jk/http-push-status-fix later to maint).\n\n * Update \"git archive\" documentation and give explicit mention on the\n   compression level for both zip and tar.gz format.\n   (merge c4b208c309 bs/archive-doc-compression-level later to maint).\n\n * Drop \"git sparse-checkout\" from the list of common commands.\n   (merge 6a9a50a8af sg/sparse-index-not-that-common-a-command later to maint).\n\n * \"git branch -c/-m new old\" was not described to copy config, which\n   has been corrected.\n   (merge 8252ec300e jc/branch-copy-doc later to maint).\n\n * Squelch over-eager warning message added during this cycle.\n   (merge 9e8fe7b1c7 jk/log-warn-on-bogus-encoding later to maint).\n\n * Fix long-standing shell syntax error in the completion script.\n   (merge 46b0585286 re/completion-fix-test-equality later to maint).\n\n * Teach \"git commit-graph\" command not to allow using replace objects\n   at all, as we do not use the commit-graph at runtime when we see\n   object replacement.\n   (merge 095d112f8c ab/ignore-replace-while-working-on-commit-graph later to maint).\n\n * \"git pull --no-verify\" did not affect the underlying \"git merge\".\n   (merge 47bfdfb3fd ar/fix-git-pull-no-verify later to maint).\n\n * One CI task based on Fedora image noticed a not-quite-kosher\n   consturct recently, which has been corrected.\n   (merge 4b540cf913 vd/pthread-setspecific-g11-fix later to maint).\n\n * Other code cleanup, docfix, build fix, etc.\n   (merge f188160be9 ab/bundle-remove-verbose-option later to maint).\n   (merge 8c6b4332b4 rs/close-pack-leakfix later to maint).\n   (merge 51b04c05b7 bs/difftool-msg-tweak later to maint).\n   (merge dd20e4a6db ab/make-compdb-fix later to maint).\n   (merge 6ffb990dc4 os/status-docfix later to maint).\n   (merge 100c2da2d3 rs/p3400-lose-tac later to maint).\n   (merge 76f3b69896 tb/aggregate-ignore-leading-whitespaces later to maint).\n   (merge 6e4fd8bfcd tz/doc-link-to-bundle-format-fix later to maint).\n   (merge f6c013dfa1 jc/doc-commit-header-continuation-line later to maint).\n   (merge ec9a37d69b ab/pkt-line-cleanup later to maint).\n   (merge 8650c6298c ab/fix-make-lint-docs later to maint).\n   (merge 1c720357ce ab/test-lib-diff-cleanup later to maint).\n   (merge 6b615dbece ks/submodule-add-message-fix later to maint).\n   (merge 82a57cd13f ma/doc-git-version later to maint).\n   (merge 203eb8381a jc/doc-format-patch-clarify-auto-base later to maint).\n   (merge 559664c792 ab/test-lib later to maint).\n"},{"id":"440810","messageId":"YYtbdkLsCSFFE5io@coredump.intra.peff.net","threadId":"56863","inReplyTo":"xmqq4k8kzuz2.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T05:41:10Z","receivedAt":"2021-11-10T05:41:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:\n\n>  * Use ssh public crypto for object and push-cert signing.\n\nI'm seeing some test breakage from the release candidates here. On my\nDebian unstable system, everything passed a few days ago. But after\nupgrading openssh-client from 1:8.4p1-5 to 1:8.7p1-1 (which hit unstable\non Saturday), all of the GPGSSH bits seem to break:\n\n  Test Summary Report\n  -------------------\n  t5534-push-signed.sh                             (Wstat: 256 Tests: 13 Failed: 2)\n    Failed tests:  8, 12\n    Non-zero exit status: 1\n  t6200-fmt-merge-msg.sh                           (Wstat: 256 Tests: 31 Failed: 2)\n    Failed tests:  7-8\n    Non-zero exit status: 1\n  t7031-verify-tag-signed-ssh.sh                   (Wstat: 256 Tests: 8 Failed: 5)\n    Failed tests:  2, 4-7\n    Non-zero exit status: 1\n  t7528-signed-commit-ssh.sh                       (Wstat: 256 Tests: 23 Failed: 10)\n    Failed tests:  2-5, 7, 9, 12-13, 17, 19\n    Non-zero exit status: 1\n\nThis doesn't have anything to do with -rc2 in particular. The breakage\nbisects to f265f2d630 (ssh signing: tests for logs, tags & push certs,\n2021-09-10), and is triggered by the system openssh upgrade.\n\nIt's hard to tell what's going on, as we seem to just be getting bad\nresults from ssh-keygen. Here's the first failing test in t7031 (with\nGIT_TRACE and -x):\n\n  [...]\n  + git verify-tag initial\n  trace: built-in: git verify-tag initial\n  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\n  trace: run_command: ssh-keygen -Y check-novalidate -n git -s /tmp/.git_vtag_tmpSxXLIv\n  + exit 1\n  error: last command exited with $?=1\n  not ok 2 - verify and show ssh signatures\n\nLikewise, this segfault (!) from t7528 is scary:\n\n  [...]\n  + git verify-commit initial\n  trace: built-in: git verify-commit initial\n  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\n  error: ssh-keygen died of signal 11\n  trace: run_command: ssh-keygen -Y check-novalidate -n git -s /tmp/.git_vtag_tmpCOAwhp\n  Good \"git\" signature with ED25519 key SHA256:E+1Xptv1zGa2fWFjSL36Tl2m2NVxcyJVzhfQTnU+yWc\n  + exit 1\n  error: last command exited with $?=1\n  not ok 2 - verify and show signatures\n\nSo it may not be a bug we need to fix in Git. But shipping v2.34 with\nlots of test failures may cause some headaches. Maybe we need to tighten\nup the GPGSSH prereq checks to block broken versions?\n\n-Peff\n"},{"id":"440811","messageId":"YYtgD8VT/0vuIHRX@coredump.intra.peff.net","threadId":"56863","inReplyTo":"YYtbdkLsCSFFE5io@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T06:00:47Z","receivedAt":"2021-11-10T06:00:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 12:41:11AM -0500, Jeff King wrote:\n\n> So it may not be a bug we need to fix in Git. But shipping v2.34 with\n> lots of test failures may cause some headaches. Maybe we need to tighten\n> up the GPGSSH prereq checks to block broken versions?\n\nThis is what I came up with, but I'm not sure it there's a better way to\nfind the broken version. I don't think there's a way to get a version\nnumber out of ssh-keygen (and anyway, checking the behavior of the\ncommand is a more robust test). My fears are:\n\n  - this does cause several segfaults per test run on affected\n    platforms, which will pollute the kernel log, etc.\n\n  - we're not really testing the desired behavior, just looking for a\n    known-problem. The segfault may get fixed but we'd still have other\n    bugs.\n\nSo it would be nice to have a more exact test, but without understanding\nthe openssh bug, I think this is the best we can do in the meantime.\n\n-- >8 --\nSubject: [PATCH] t/lib-gpg: avoid broken versions of ssh-keygen\n\nThe \"-Y find-principals\" option of ssh-keygen seems to be broken in\nDebian's openssh-client 1:8.7p1-1, whereas it works fine in 1:8.4p1-5.\nThis causes several failures for GPGSSH tests. We fulfill the\nprerequisite because generating the keys works fine, but actually\nverifying a signature causes results ranging from bogus results to\nssh-keygen segfaulting.\n\nWe can find the broken version during the prereq check by feeding it\nempty input. This should result in it complaining to stderr, but in the\nbroken version it triggers the segfault, causing the GPGSSH tests to be\nskipped.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/lib-gpg.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 1d8e5b5b7e..a3f285f515 100644\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -104,6 +104,12 @@ test_lazy_prereq GPGSSH '\n \ttest $? != 127 || exit 1\n \techo $ssh_version | grep -q \"find-principals:missing signature file\"\n \ttest $? = 0 || exit 1;\n+\n+\t# some broken versions of ssh-keygen segfault on find-principals;\n+\t# avoid testing with them.\n+\tssh-keygen -Y find-principals -f /dev/null -s /dev/null\n+\ttest $? = 139 && exit 1\n+\n \tmkdir -p \"${GNUPGHOME}\" &&\n \tchmod 0700 \"${GNUPGHOME}\" &&\n \t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n-- \n2.34.0.rc1.634.g85d556ea55\n\n"},{"id":"440812","messageId":"20211110063547.xcpjjzoobha3eeff@gmail.com","threadId":"56863","inReplyTo":"xmqq4k8kzuz2.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-11-10T06:35:47Z","receivedAt":"2021-11-10T06:35:53Z","isPatch":false,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:\n> A release candidate Git v2.34.0-rc2 is now available for testing\n> \n> Fixes since v2.33\n> -----------------\n> \n>  * Doc update plus improved error reporting.\n\nThis should be something like\n\n * Warn when iconv(3) fails to reencode log messages.\n\n> \n>  * Recent \"diff -m\" changes broke \"gitk\", which has been corrected.\n> \n>  * Regression fix.\n\nI assume that generic merge messages like \"Regression fix.\" or \"Doc update.\"\nare meant to be dropped or replaced, maybe\n\n  * Fix a v2.33.0 regression where \"git send-email\" would use wrong values for\n    sendemail.* options that are defined in multiple configuration files.\n\n>  * One CI task based on Fedora image noticed a not-quite-kosher\n>    consturct recently, which has been corrected.\n\nI guess consturct -> construct\n\n>    (merge 4b540cf913 vd/pthread-setspecific-g11-fix later to maint).\n"},{"id":"440814","messageId":"CAPUEspg+NDJFAVcFs2hhcSTORb0Q-Gjcv0M+5tzqXJuBTvbT+Q@mail.gmail.com","threadId":"56863","inReplyTo":"YYtgD8VT/0vuIHRX@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-10T08:11:12Z","receivedAt":"2021-11-10T08:11:26Z","isPatch":false,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:\n\n>   - we're not really testing the desired behavior, just looking for a\n>     known-problem. The segfault may get fixed but we'd still have other\n>     bugs.\n\nThis openssh bug was fixed in 8.8 per the release notes; indeed the\nfix[1] (which was misapplied but fixed next commit) looks familiar and\nit is just a straight up crasher, hence unlikely to cause other\nissues.\n\nAgree with you though that with both git and openssh getting changes\nreleased for this feature there is a probability that there might be\nmisbehaving combinations that are not appropriately tested, but your\npatch is a good step in the right direction.\n\nCarlo\n\n[1] https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/ssh-keygen.c.diff?r1=1.435&r2=1.436&f=h\n"},{"id":"440815","messageId":"YYuBQ2mUCeEmYoLR@coredump.intra.peff.net","threadId":"56863","inReplyTo":"CAPUEspg+NDJFAVcFs2hhcSTORb0Q-Gjcv0M+5tzqXJuBTvbT+Q@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T08:22:27Z","receivedAt":"2021-11-10T08:22:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:\n\n> On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:\n> \n> >   - we're not really testing the desired behavior, just looking for a\n> >     known-problem. The segfault may get fixed but we'd still have other\n> >     bugs.\n> \n> This openssh bug was fixed in 8.8 per the release notes; indeed the\n> fix[1] (which was misapplied but fixed next commit) looks familiar and\n> it is just a straight up crasher, hence unlikely to cause other\n> issues.\n\nAh, thanks for digging. I agree that this is a small isolated bug, so\nthe prereq check I showed would be a good test for it.\n\nIMHO it's worth doing. It looks like 8.7 is the only affected openssh\nversion, but it is likely to cause confusion right when we release.\n\n-Peff\n"},{"id":"440816","messageId":"YYuBY3jIjqrMxNPJ@coredump.intra.peff.net","threadId":"56863","inReplyTo":"20211110063547.xcpjjzoobha3eeff@gmail.com","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T08:22:59Z","receivedAt":"2021-11-10T08:23:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 07:35:47AM +0100, Johannes Altmanninger wrote:\n\n> > Fixes since v2.33\n> > -----------------\n> > \n> >  * Doc update plus improved error reporting.\n> \n> This should be something like\n> \n>  * Warn when iconv(3) fails to reencode log messages.\n\nYes, though note that we ended up reverting the warning. So probably it\ncould be removed from the release notes entirely.\n\n-Peff\n"},{"id":"440818","messageId":"CAPUEspjzs0ZvDGUwp=XYxdHb_t+1yx0C1epaNhhiAn6SsPeidQ@mail.gmail.com","threadId":"56863","inReplyTo":"YYuBQ2mUCeEmYoLR@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-10T09:15:52Z","receivedAt":"2021-11-10T09:16:06Z","isPatch":false,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 10, 2021 at 12:22 AM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:\n>\n> > On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:\n> >\n> > >   - we're not really testing the desired behavior, just looking for a\n> > >     known-problem. The segfault may get fixed but we'd still have other\n> > >     bugs.\n> >\n> > This openssh bug was fixed in 8.8 per the release notes; indeed the\n> > fix[1] (which was misapplied but fixed next commit) looks familiar and\n> > it is just a straight up crasher, hence unlikely to cause other\n> > issues.\n>\n> Ah, thanks for digging. I agree that this is a small isolated bug, so\n> the prereq check I showed would be a good test for it.\n>\n> IMHO it's worth doing. It looks like 8.7 is the only affected openssh\n> version, but it is likely to cause confusion right when we release.\n\nIt was discussed[1] before, and I think the plan was to at least\nmention the brokenness with 8.7 in the release notes, but guess I\ndropped the ball by not raising it earlier.\n\nCarlo\n\n[1] https://lore.kernel.org/git/CAPUEspgnRFNRoFuEvP1hpY3iKukk3OnF4zk85wkdkmiVuPuRTw@mail.gmail.com/\n"},{"id":"440821","messageId":"YYuSYz+XFWmBkIiz@coredump.intra.peff.net","threadId":"56863","inReplyTo":"CAPUEspjzs0ZvDGUwp=XYxdHb_t+1yx0C1epaNhhiAn6SsPeidQ@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-10T09:35:31Z","receivedAt":"2021-11-10T09:35:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 01:15:52AM -0800, Carlo Arenas wrote:\n\n> On Wed, Nov 10, 2021 at 12:22 AM Jeff King <peff@peff.net> wrote:\n> >\n> > On Wed, Nov 10, 2021 at 12:11:12AM -0800, Carlo Arenas wrote:\n> >\n> > > On Tue, Nov 9, 2021 at 10:02 PM Jeff King <peff@peff.net> wrote:\n> > >\n> > > >   - we're not really testing the desired behavior, just looking for a\n> > > >     known-problem. The segfault may get fixed but we'd still have other\n> > > >     bugs.\n> > >\n> > > This openssh bug was fixed in 8.8 per the release notes; indeed the\n> > > fix[1] (which was misapplied but fixed next commit) looks familiar and\n> > > it is just a straight up crasher, hence unlikely to cause other\n> > > issues.\n> >\n> > Ah, thanks for digging. I agree that this is a small isolated bug, so\n> > the prereq check I showed would be a good test for it.\n> >\n> > IMHO it's worth doing. It looks like 8.7 is the only affected openssh\n> > version, but it is likely to cause confusion right when we release.\n> \n> It was discussed[1] before, and I think the plan was to at least\n> mention the brokenness with 8.7 in the release notes, but guess I\n> dropped the ball by not raising it earlier.\n\nAh, thanks. I looked in the archive, but assumed any mention would have\nbeen more recent (not realizing that Debian's packaging was simply\nlagging the upstream releases by quite a bit).\n\n-Peff\n"},{"id":"440822","messageId":"20211110093912.4382-1-carenas@gmail.com","threadId":"56863","inReplyTo":"YYtgD8VT/0vuIHRX@coredump.intra.peff.net","subject":"[PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-11-10T09:39:12Z","receivedAt":"2021-11-10T09:39:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"As discussed[1] earlier, make sure there are no surprises when ssh-keygen\ncrashes on some users of OpenSSH 8.7 that are trying ssh signing.\n\n[1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Documentation/RelNotes/2.34.0.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt\nindex effab2ea4b..54dcc7240d 100644\n--- a/Documentation/RelNotes/2.34.0.txt\n+++ b/Documentation/RelNotes/2.34.0.txt\n@@ -8,6 +8,9 @@ Backward compatibility notes\n \n  * The \"--preserve-merges\" option of \"git rebase\" has been removed.\n \n+ * The upcoming ssh signing feature is broken if used together with\n+   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8\n+   (or stay at 8.6)\n \n UI, Workflows & Features\n \n-- \n2.34.0.rc1.349.g8f33748433\n\n"},{"id":"440876","messageId":"xmqqczn7y9kh.fsf@gitster.g","threadId":"56863","inReplyTo":"20211110093912.4382-1-carenas@gmail.com","subject":"Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T21:39:26Z","receivedAt":"2021-11-10T21:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> As discussed[1] earlier, make sure there are no surprises when ssh-keygen\n> crashes on some users of OpenSSH 8.7 that are trying ssh signing.\n>\n> [1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  Documentation/RelNotes/2.34.0.txt | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt\n> index effab2ea4b..54dcc7240d 100644\n> --- a/Documentation/RelNotes/2.34.0.txt\n> +++ b/Documentation/RelNotes/2.34.0.txt\n> @@ -8,6 +8,9 @@ Backward compatibility notes\n>  \n>   * The \"--preserve-merges\" option of \"git rebase\" has been removed.\n>  \n> + * The upcoming ssh signing feature is broken if used together with\n> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8\n> +   (or stay at 8.6)\n\nThat may be correct, but it is NOT a backward compatibility note.\n"},{"id":"440877","messageId":"xmqq8rxvy9d3.fsf@gitster.g","threadId":"56863","inReplyTo":"YYuBY3jIjqrMxNPJ@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T21:43:52Z","receivedAt":"2021-11-10T21:43:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 10, 2021 at 07:35:47AM +0100, Johannes Altmanninger wrote:\n>\n>> > Fixes since v2.33\n>> > -----------------\n>> > \n>> >  * Doc update plus improved error reporting.\n>> \n>> This should be something like\n>> \n>>  * Warn when iconv(3) fails to reencode log messages.\n>\n> Yes, though note that we ended up reverting the warning. So probably it\n> could be removed from the release notes entirely.\n\nTrue.  Thanks for carefully reading these entries, all.\n\n"},{"id":"440878","messageId":"xmqq1r3ny932.fsf@gitster.g","threadId":"56863","inReplyTo":"YYtgD8VT/0vuIHRX@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T21:49:53Z","receivedAt":"2021-11-10T21:49:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So it would be nice to have a more exact test, but without understanding\n> the openssh bug, I think this is the best we can do in the meantime.\n\nSounds good.  We can also ensure that the key we are going to\ngenerate here is actually usable before we leave the lazy-prereq\nblock, but we can leave it for another day.\n\nWill apply.  Thanks.\n\n\n>\n> -- >8 --\n> Subject: [PATCH] t/lib-gpg: avoid broken versions of ssh-keygen\n>\n> The \"-Y find-principals\" option of ssh-keygen seems to be broken in\n> Debian's openssh-client 1:8.7p1-1, whereas it works fine in 1:8.4p1-5.\n> This causes several failures for GPGSSH tests. We fulfill the\n> prerequisite because generating the keys works fine, but actually\n> verifying a signature causes results ranging from bogus results to\n> ssh-keygen segfaulting.\n>\n> We can find the broken version during the prereq check by feeding it\n> empty input. This should result in it complaining to stderr, but in the\n> broken version it triggers the segfault, causing the GPGSSH tests to be\n> skipped.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/lib-gpg.sh | 6 ++++++\n>  1 file changed, 6 insertions(+)\n>\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index 1d8e5b5b7e..a3f285f515 100644\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -104,6 +104,12 @@ test_lazy_prereq GPGSSH '\n>  \ttest $? != 127 || exit 1\n>  \techo $ssh_version | grep -q \"find-principals:missing signature file\"\n>  \ttest $? = 0 || exit 1;\n> +\n> +\t# some broken versions of ssh-keygen segfault on find-principals;\n> +\t# avoid testing with them.\n> +\tssh-keygen -Y find-principals -f /dev/null -s /dev/null\n> +\ttest $? = 139 && exit 1\n> +\n>  \tmkdir -p \"${GNUPGHOME}\" &&\n>  \tchmod 0700 \"${GNUPGHOME}\" &&\n>  \t(setfacl -k \"${GNUPGHOME}\" 2>/dev/null || true) &&\n"},{"id":"440880","messageId":"xmqqmtmbwtim.fsf@gitster.g","threadId":"56863","inReplyTo":"xmqqczn7y9kh.fsf@gitster.g","subject":"Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-10T22:11:29Z","receivedAt":"2021-11-10T22:14:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n>\n>> As discussed[1] earlier, make sure there are no surprises when ssh-keygen\n>> crashes on some users of OpenSSH 8.7 that are trying ssh signing.\n>>\n>> [1] https://lore.kernel.org/git/xmqqsfycs21q.fsf@gitster.g/\n>>\n>> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n>> ---\n>>  Documentation/RelNotes/2.34.0.txt | 3 +++\n>>  1 file changed, 3 insertions(+)\n>>\n>> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt\n>> index effab2ea4b..54dcc7240d 100644\n>> --- a/Documentation/RelNotes/2.34.0.txt\n>> +++ b/Documentation/RelNotes/2.34.0.txt\n>> @@ -8,6 +8,9 @@ Backward compatibility notes\n>>  \n>>   * The \"--preserve-merges\" option of \"git rebase\" has been removed.\n>>  \n>> + * The upcoming ssh signing feature is broken if used together with\n>> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8\n>> +   (or stay at 8.6)\n>\n> That may be correct, but it is NOT a backward compatibility note.\n\nSo, here is what I plan to do.\n\ndiff --git c/Documentation/RelNotes/2.34.0.txt w/Documentation/RelNotes/2.34.0.txt\nindex effab2ea4b..6ed8b92e10 100644\n--- c/Documentation/RelNotes/2.34.0.txt\n+++ w/Documentation/RelNotes/2.34.0.txt\n@@ -77,7 +77,10 @@ UI, Workflows & Features\n  * \"git fsck\" has been taught to report mismatch between expected and\n    actual types of an object better.\n \n- * Use ssh public crypto for object and push-cert signing.\n+ * In addition to GnuPG, ssh public crypto can be used for object and\n+   push-cert signing.  Note that this feature cannot be used with\n+   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid\n+   using it unless you update to OpenSSH 8.8.\n \n  * \"git log --grep=string --author=name\" learns to highlight hits just\n    like \"git grep string\" does.\n"},{"id":"440903","messageId":"YYzfhKt7SRT9p8Df@coredump.intra.peff.net","threadId":"56863","inReplyTo":"xmqqmtmbwtim.fsf@gitster.g","subject":"Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-11T09:16:52Z","receivedAt":"2021-11-11T09:16:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 10, 2021 at 02:11:29PM -0800, Junio C Hamano wrote:\n\n> >> diff --git a/Documentation/RelNotes/2.34.0.txt b/Documentation/RelNotes/2.34.0.txt\n> >> index effab2ea4b..54dcc7240d 100644\n> >> --- a/Documentation/RelNotes/2.34.0.txt\n> >> +++ b/Documentation/RelNotes/2.34.0.txt\n> >> @@ -8,6 +8,9 @@ Backward compatibility notes\n> >>  \n> >>   * The \"--preserve-merges\" option of \"git rebase\" has been removed.\n> >>  \n> >> + * The upcoming ssh signing feature is broken if used together with\n> >> +   OpenSSH 8.7, avoid using it if you cannot update to OpenSSH 8.8\n> >> +   (or stay at 8.6)\n> >\n> > That may be correct, but it is NOT a backward compatibility note.\n> \n> So, here is what I plan to do.\n> \n> diff --git c/Documentation/RelNotes/2.34.0.txt w/Documentation/RelNotes/2.34.0.txt\n> index effab2ea4b..6ed8b92e10 100644\n> --- c/Documentation/RelNotes/2.34.0.txt\n> +++ w/Documentation/RelNotes/2.34.0.txt\n> @@ -77,7 +77,10 @@ UI, Workflows & Features\n>   * \"git fsck\" has been taught to report mismatch between expected and\n>     actual types of an object better.\n>  \n> - * Use ssh public crypto for object and push-cert signing.\n> + * In addition to GnuPG, ssh public crypto can be used for object and\n> +   push-cert signing.  Note that this feature cannot be used with\n> +   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid\n> +   using it unless you update to OpenSSH 8.8.\n\nAttaching to the existing gpg-ssh release note like this makes perfect\nsense to me. Thanks for tying this one up.\n\n-Peff\n"},{"id":"440908","messageId":"YY0HbQJEWbOwuuFj@coredump.intra.peff.net","threadId":"56863","inReplyTo":"xmqq4k8kzuz2.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-11T12:07:09Z","receivedAt":"2021-11-11T12:07:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:\n\n>  * The revision traversal API has been optimized by taking advantage\n>    of the commit-graph, when available, to determine if a commit is\n>    reachable from any of the existing refs.\n\nI was thinking a bit about this change, specifically f45022dc2f\n(connected: do not sort input revisions, 2021-08-09). At the time, Junio\nsaid[1]:\n\n  Sorting of positive side is done to help both performance and\n  correctness in regular use of the traversal machinery, especially\n  when reachability bitmap is not in effect, but on the negative side\n  I do not think there is any downside to omit sorting offhand.  The\n  only case that may get affected is when the revision.c::SLOP kicks\n  in to deal with oddball commits with incorrect committer timestamps,\n  but then the result of the sorting isn't to be trusted anyway, so...\n\nBut wouldn't failure to sort the commits actually _create_ such an\n\"oddball commits\" scenario, because we'd see the UNINTERESTING commits\nin an arbitrary order, especially with respect to the interesting ones?\n\nI wondered if there were cases where this would change the output of the\ncommand, and came up with this:\n\n-- >8 --\ngit init -q repo\ncd repo\n\n# make a commit whose timestamp is offset from an arbitrary start point\n# by $1 days. That gives us a deterministic commit ordering with no\n# chance of any time-based slop.\ncommit_at() {\n\tbase=1234567890\n\tunit=86400\n\ttimestamp=\"@$((base + $1 * unit)) +0000\" \\\n\tGIT_COMMITTER_DATE=$timestamp \\\n\tGIT_AUTHOR_DATE=$timestamp \\\n\tgit commit --allow-empty -qm \"commit at day $1\"\n\tgit tag day-$1\n}\ncommit_at 1\ncommit_at 2\ncommit_at 3\ncommit_at 4\ncommit_at 5\ncommit_at 6\ncommit_at 7\ncommit_at 8\ncommit_at 9\ncommit_at 10\n\n# We ask to see day-1 and day-10, but the limit_list() traversal\n# should drop day-1 as an ancestor of the others. There are two\n# important things about the negative revisions:\n#\n#  - there are more than 5 of them, which will run out the SLOP counter\n#\n#  - we don't include ^day-2, as that could cause us to directly mark\n#    its parent day-1 as UNINTERESTING\n#\n# Obviously this command line is contrived, but you could get the same\n# inputs from a \"rev-list $new_stuff --not --all\" connectivity check,\n# depending on where refs were pointing and what names they had.\ntips='day-1 day-10 ^day-9 ^day-8 ^day-7 ^day-6 ^day-5 ^day-4'\n\n# Now we can compare the sorted/unsorted inputs. I used \"log\" here just\n# to make the output easier to understand.\ngit log --oneline $tips >expect\ngit log --oneline --unsorted-input $tips >actual\ndiff -u expect actual\n-- >8 --\n\nAnd the output I get is:\n\n  --- expect\t2021-11-11 06:50:22.148050216 -0500\n  +++ actual\t2021-11-11 06:50:22.152050263 -0500\n  @@ -1 +1,2 @@\n  +027d297 commit at day 1\n   1dc264f commit at day 10\n\nSo indeed, we get confused by the unsorted input and give the wrong\nanswer. We should look at and propagate UNINTERESTING marks from day-9,\netc on down to day-1, but we don't.\n\nNow in this case, we're sending too much output, which is OK for the\npurposes of the connectivity check. It will just walk day-1 and its\ntree unnecessarily, which is a performance loss but not incorrect.\n\nMy intuition is that this is _probably_ the only type of incorrect\nanswer we'd give (i.e., that we'd never go the other way, omitting an\nobject we should have included) because nobody would ever set the\nUNINTERESTING flag unnecessarily, and everybody_uninteresting() does do\na full scan looking for any positive tips.\n\nStill, I didn't see this subtlety mentioned in the earlier discussion,\nand it's a bit alarming not to understand all of the possible\nimplications. I'm not sure if it rises to the level of something we want\nto consider more or address before the release. Sorry to only come up\nwith this at the -rc2 stage, but I figure now is better than right after\nthe release. ;)\n\n-Peff\n\n[1] https://lore.kernel.org/git/xmqqa6lzwu31.fsf@gitster.g/\n"},{"id":"440919","messageId":"xmqqwnlemwcy.fsf@gitster.g","threadId":"56863","inReplyTo":"YY0HbQJEWbOwuuFj@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T17:32:29Z","receivedAt":"2021-11-11T17:32:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Nov 09, 2021 at 04:59:29PM -0800, Junio C Hamano wrote:\n>\n>>  * The revision traversal API has been optimized by taking advantage\n>>    of the commit-graph, when available, to determine if a commit is\n>>    reachable from any of the existing refs.\n>\n> I was thinking a bit about this change, specifically f45022dc2f\n> (connected: do not sort input revisions, 2021-08-09). At the time, Junio\n> said[1]:\n>\n>   Sorting of positive side is done to help both performance and\n>   correctness in regular use of the traversal machinery, especially\n>   when reachability bitmap is not in effect, but on the negative side\n>   I do not think there is any downside to omit sorting offhand.  The\n>   only case that may get affected is when the revision.c::SLOP kicks\n>   in to deal with oddball commits with incorrect committer timestamps,\n>   but then the result of the sorting isn't to be trusted anyway, so...\n>\n> But wouldn't failure to sort the commits actually _create_ such an\n> \"oddball commits\" scenario, because we'd see the UNINTERESTING commits\n> in an arbitrary order, especially with respect to the interesting ones?\n\nYou're right.\n\n> So indeed, we get confused by the unsorted input and give the wrong\n> answer. We should look at and propagate UNINTERESTING marks from day-9,\n> etc on down to day-1, but we don't.\n>\n> Now in this case, we're sending too much output, which is OK for the\n> purposes of the connectivity check. It will just walk day-1 and its\n> tree unnecessarily, which is a performance loss but not incorrect.\n\nThe primary use of the traversal in check_connected() is when we\nhave new refs we haven't seen, and they go to the positive end of\nthe traversal, which will end in the refs we do have (there may be\ntons).  The idea is that when one or more of the new refs are truly\n\"new\" in the sense that one or more the objects necessary to connect\nthem to our refs do not exist, not even in the \"not reachable but\nnot yet pruned\" state, this traversal will hit a missing object and\nwill error out.  So, it is alarming that \"day-1\" is shown without\npainted uninteresting via any of the negative [day-4..day-9]\ncommits.  Which means, if we are checking if we need to initiate a\nreal fetch to connect day-1 and day-10 to our DAG, when we think we\nhave [day-4..day-9] and everything behind them, we stopped traversal\nbefore seeing all the \"objects necessary to connect them to our\nrefs\".  If day-2 were missing in our repository, we would have\nnoticed if we did traversal from sorted tips, but the unsorted\ntraversal happily misses it.\n\nNot that noticing that day-2 is missing from our repository does not\nhelp much in *this* particular case, though.  It is likely that a\ncommon negotiation would say \"I have [day-4..day-9], and want day-1\nand day-10\", that is reponded with \"OK, I know enough, here is\nday-10 and its tree/blob that would be missing from a healthy clone\nwith everything behind day-9\", and it won't include day-2 (nor\nday-1).  So in this particular example, it would not matter if the\nnew unsorted traversal is subtly broken (I think the extent of the\ndamage is similar to making the SLOP problem deliberately worse),\nbut I am not sure if there are other failure modes that would yield\noutright incorrect result.\n\n> My intuition is that this is _probably_ the only type of incorrect\n> answer we'd give (i.e., that we'd never go the other way, omitting an\n> object we should have included) because nobody would ever set the\n> UNINTERESTING flag unnecessarily, and everybody_uninteresting() does do\n> a full scan looking for any positive tips.\n>\n> Still, I didn't see this subtlety mentioned in the earlier discussion,\n> and it's a bit alarming not to understand all of the possible\n> implications. I'm not sure if it rises to the level of something we want\n> to consider more or address before the release. Sorry to only come up\n> with this at the -rc2 stage, but I figure now is better than right after\n> the release. ;)\n>\n> -Peff\n>\n> [1] https://lore.kernel.org/git/xmqqa6lzwu31.fsf@gitster.g/\n\nWe probably should revert this step as it can affect correctness in\na big way, but I wonder if the other steps in the same series, or\nother topic that came later, rely on it.\n\nAt the very least, I think this may be prudent during -rc period,\nbut on the other hand, I do not know offhand what would later\npursuade us to reinstate it and convince us that it is a safe thing\nto do.\n\n connected.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git c/connected.c w/connected.c\nindex cf68e37a97..35bd4a2638 100644\n--- c/connected.c\n+++ w/connected.c\n@@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tif (opt->progress)\n \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t     _(\"Checking connectivity\"));\n-\tstrvec_push(&rev_list.args, \"--unsorted-input\");\n \n \trev_list.git_cmd = 1;\n \trev_list.env = opt->env;\n"},{"id":"440934","messageId":"YY17rBFIdDl+H47I@coredump.intra.peff.net","threadId":"56863","inReplyTo":"xmqqwnlemwcy.fsf@gitster.g","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-11T20:23:08Z","receivedAt":"2021-11-11T20:23:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:\n\n> > Now in this case, we're sending too much output, which is OK for the\n> > purposes of the connectivity check. It will just walk day-1 and its\n> > tree unnecessarily, which is a performance loss but not incorrect.\n> \n> The primary use of the traversal in check_connected() is when we\n> have new refs we haven't seen, and they go to the positive end of\n> the traversal, which will end in the refs we do have (there may be\n> tons).  The idea is that when one or more of the new refs are truly\n> \"new\" in the sense that one or more the objects necessary to connect\n> them to our refs do not exist, not even in the \"not reachable but\n> not yet pruned\" state, this traversal will hit a missing object and\n> will error out.  So, it is alarming that \"day-1\" is shown without\n> painted uninteresting via any of the negative [day-4..day-9]\n> commits.  Which means, if we are checking if we need to initiate a\n> real fetch to connect day-1 and day-10 to our DAG, when we think we\n> have [day-4..day-9] and everything behind them, we stopped traversal\n> before seeing all the \"objects necessary to connect them to our\n> refs\".  If day-2 were missing in our repository, we would have\n> noticed if we did traversal from sorted tips, but the unsorted\n> traversal happily misses it.\n\nYes, but if day-2 were missing in our repository, then we are already\ncorrupt. And in most cases we would not notice adding a new ref that is\nalso corrupt (e.g., imagine adding _just_ day-10 which is a descendent\nof day-2, but we stop traversal at day-9 when we see that we already\nhave it reachable).\n\nSo I don't think it is actually changing the check_connected() outcome.\nI couldn't come up with a case where we should be checking a commit and\ndon't. Only the other way around.\n\n> Not that noticing that day-2 is missing from our repository does not\n> help much in *this* particular case, though.  It is likely that a\n> common negotiation would say \"I have [day-4..day-9], and want day-1\n> and day-10\", that is reponded with \"OK, I know enough, here is\n> day-10 and its tree/blob that would be missing from a healthy clone\n> with everything behind day-9\", and it won't include day-2 (nor\n> day-1).  So in this particular example, it would not matter if the\n> new unsorted traversal is subtly broken (I think the extent of the\n> damage is similar to making the SLOP problem deliberately worse),\n> but I am not sure if there are other failure modes that would yield\n> outright incorrect result.\n\nYes, I think that framing is right: it is making SLOP much worse. We\ncould similarly have had bogus timestamps in those commits which would\ncause the same outcome. So in that sense it is nothing new. On the other\nhand, I wonder how often it will cause extra traversal work (keeping in\nmind that this commit traversal is just the first stage; after we find\nthe commits, then we talk all of their trees, which is the more\nexpensive part).\n\nFor the case of adding new commits directly on top of another branch, I\nthink there would be no change. But any time you have to walk down to a\ncommon fork point (e.g., imagine I made a new branch forked from an old\nbit of history), we may fail to find that. I haven't quite constructed\nan example, but I have a feeling we could end up walking over\narbitrarily long segments of history.\n\n> We probably should revert this step as it can affect correctness in\n> a big way, but I wonder if the other steps in the same series, or\n> other topic that came later, rely on it.\n\nI looked them over, and I think this is pretty independent (with the\nexception of the refactoring of the no_walk/unsorted flags, but\nobviously that had to come first).\n\n> At the very least, I think this may be prudent during -rc period,\n> but on the other hand, I do not know offhand what would later\n> pursuade us to reinstate it and convince us that it is a safe thing\n> to do.\n> [...]\n> diff --git c/connected.c w/connected.c\n> index cf68e37a97..35bd4a2638 100644\n> --- c/connected.c\n> +++ w/connected.c\n> @@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \tif (opt->progress)\n>  \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n>  \t\t\t     _(\"Checking connectivity\"));\n> -\tstrvec_push(&rev_list.args, \"--unsorted-input\");\n>  \n>  \trev_list.git_cmd = 1;\n>  \trev_list.env = opt->env;\n\nThis seems like a pretty safe and minimal backing-out for the -rc\nperiod. We would still ship with \"--unsorted-input\" as an option (and\nmentioned in the docs), though. If we're worried that it might be a dead\nend and we don't want to support it, we could revert f45022dc2f\n(connected: do not sort input revisions, 2021-08-09) entirely. That\ncarries a little more risk of accidentally breaking something during the\nrevert, but from my reading of the patch it should be pretty safe.\n\nI'd be curious to hear Patrick's thoughts on the whole thing.\n\n-Peff\n"},{"id":"440937","messageId":"xmqqy25ujuv4.fsf@gitster.g","threadId":"56863","inReplyTo":"YY17rBFIdDl+H47I@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-11T20:33:03Z","receivedAt":"2021-11-11T20:33:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:\n>\n>> ...  So in this particular example, it would not matter if the\n>> new unsorted traversal is subtly broken (I think the extent of the\n>> damage is similar to making the SLOP problem deliberately worse),\n>> but I am not sure if there are other failure modes that would yield\n>> outright incorrect result.\n>\n> Yes, I think that framing is right: it is making SLOP much worse. We\n> could similarly have had bogus timestamps in those commits which would\n> cause the same outcome. So in that sense it is nothing new. On the other\n> hand, I wonder how often it will cause extra traversal work (keeping in\n> mind that this commit traversal is just the first stage; after we find\n> the commits, then we talk all of their trees, which is the more\n> expensive part).\n>\n> For the case of adding new commits directly on top of another branch, I\n> think there would be no change. But any time you have to walk down to a\n> common fork point (e.g., imagine I made a new branch forked from an old\n> bit of history), we may fail to find that. I haven't quite constructed\n> an example, but I have a feeling we could end up walking over\n> arbitrarily long segments of history.\n> ...\n> I'd be curious to hear Patrick's thoughts on the whole thing.\n\nYes.  I'm tempted to wait for him to chime in.\n"},{"id":"441041","messageId":"xmqqh7cgoqoj.fsf@gitster.g","threadId":"56863","inReplyTo":"YY7/peK1EOHtATEI@camp.crustytoothpaste.net","subject":"Re: [PATCH] RelNotes: mention known crasher when ssh signing with OpenSSH 8.7","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-13T00:16:44Z","receivedAt":"2021-11-13T00:16:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On 2021-11-11 at 09:16:52, Jeff King wrote:\n>> On Wed, Nov 10, 2021 at 02:11:29PM -0800, Junio C Hamano wrote:\n>>  ....\n>> > + * In addition to GnuPG, ssh public crypto can be used for object and\n>> > +   push-cert signing.  Note that this feature cannot be used with\n>> > +   ssh-keygen from OpenSSH 8.7, whose support for it is broken.  Avoid\n>> > +   using it unless you update to OpenSSH 8.8.\n>> \n>> Attaching to the existing gpg-ssh release note like this makes perfect\n>> sense to me. Thanks for tying this one up.\n>\n> Since this now affects running the testsuite on Debian unstable, we\n> probably need to also fix the testsuite such that the GPGSSH\n> prerequisite fails if we're using OpenSSH 8.7 so that developers and\n> distros aren't negatively affected.\n\nYes, I think YYtgD8VT/0vuIHRX@coredump.intra.peff.net is a good\nthing to have, which is ca7a5bf4 (t/lib-gpg: avoid broken versions\nof ssh-keygen, 2021-11-10) that was merged a few days ago.\n\nThanks for being extra careful.\n\n\n\n"},{"id":"441138","messageId":"YZJ3hZkX9YjNBAdk@ncase","threadId":"56863","inReplyTo":"YY17rBFIdDl+H47I@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-11-15T15:06:45Z","receivedAt":"2021-11-15T15:07:25Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 11, 2021 at 03:23:08PM -0500, Jeff King wrote:\n> On Thu, Nov 11, 2021 at 09:32:29AM -0800, Junio C Hamano wrote:\n> \n> > > Now in this case, we're sending too much output, which is OK for the\n> > > purposes of the connectivity check. It will just walk day-1 and its\n> > > tree unnecessarily, which is a performance loss but not incorrect.\n> > \n> > The primary use of the traversal in check_connected() is when we\n> > have new refs we haven't seen, and they go to the positive end of\n> > the traversal, which will end in the refs we do have (there may be\n> > tons).  The idea is that when one or more of the new refs are truly\n> > \"new\" in the sense that one or more the objects necessary to connect\n> > them to our refs do not exist, not even in the \"not reachable but\n> > not yet pruned\" state, this traversal will hit a missing object and\n> > will error out.  So, it is alarming that \"day-1\" is shown without\n> > painted uninteresting via any of the negative [day-4..day-9]\n> > commits.  Which means, if we are checking if we need to initiate a\n> > real fetch to connect day-1 and day-10 to our DAG, when we think we\n> > have [day-4..day-9] and everything behind them, we stopped traversal\n> > before seeing all the \"objects necessary to connect them to our\n> > refs\".  If day-2 were missing in our repository, we would have\n> > noticed if we did traversal from sorted tips, but the unsorted\n> > traversal happily misses it.\n> \n> Yes, but if day-2 were missing in our repository, then we are already\n> corrupt. And in most cases we would not notice adding a new ref that is\n> also corrupt (e.g., imagine adding _just_ day-10 which is a descendent\n> of day-2, but we stop traversal at day-9 when we see that we already\n> have it reachable).\n> \n> So I don't think it is actually changing the check_connected() outcome.\n> I couldn't come up with a case where we should be checking a commit and\n> don't. Only the other way around.\n> \n> > Not that noticing that day-2 is missing from our repository does not\n> > help much in *this* particular case, though.  It is likely that a\n> > common negotiation would say \"I have [day-4..day-9], and want day-1\n> > and day-10\", that is reponded with \"OK, I know enough, here is\n> > day-10 and its tree/blob that would be missing from a healthy clone\n> > with everything behind day-9\", and it won't include day-2 (nor\n> > day-1).  So in this particular example, it would not matter if the\n> > new unsorted traversal is subtly broken (I think the extent of the\n> > damage is similar to making the SLOP problem deliberately worse),\n> > but I am not sure if there are other failure modes that would yield\n> > outright incorrect result.\n> \n> Yes, I think that framing is right: it is making SLOP much worse. We\n> could similarly have had bogus timestamps in those commits which would\n> cause the same outcome. So in that sense it is nothing new. On the other\n> hand, I wonder how often it will cause extra traversal work (keeping in\n> mind that this commit traversal is just the first stage; after we find\n> the commits, then we talk all of their trees, which is the more\n> expensive part).\n> \n> For the case of adding new commits directly on top of another branch, I\n> think there would be no change. But any time you have to walk down to a\n> common fork point (e.g., imagine I made a new branch forked from an old\n> bit of history), we may fail to find that. I haven't quite constructed\n> an example, but I have a feeling we could end up walking over\n> arbitrarily long segments of history.\n\nSorry, but I'm currently completely loaded with work and thus didn't\nfind the capacity to have a deeper look yet and will probably not find\nthe time for a few more days. So the earliest I can have a look at this\nis probably beginning next week.\n\nWith that in mind, I'm happy to have this change reverted for now, as it\nis...\n\n> > We probably should revert this step as it can affect correctness in\n> > a big way, but I wonder if the other steps in the same series, or\n> > other topic that came later, rely on it.\n> \n> I looked them over, and I think this is pretty independent (with the\n> exception of the refactoring of the no_walk/unsorted flags, but\n> obviously that had to come first).\n\n.. completely independent of the other patches in this series and can be\nreverted on its own. Only question is whether we also want to revert the\npatch introducing this option in the first place given that it would end\nup without a user afterwards.\n\nPatrick\n\n> > At the very least, I think this may be prudent during -rc period,\n> > but on the other hand, I do not know offhand what would later\n> > pursuade us to reinstate it and convince us that it is a safe thing\n> > to do.\n> > [...]\n> > diff --git c/connected.c w/connected.c\n> > index cf68e37a97..35bd4a2638 100644\n> > --- c/connected.c\n> > +++ w/connected.c\n> > @@ -107,7 +107,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> >  \tif (opt->progress)\n> >  \t\tstrvec_pushf(&rev_list.args, \"--progress=%s\",\n> >  \t\t\t     _(\"Checking connectivity\"));\n> > -\tstrvec_push(&rev_list.args, \"--unsorted-input\");\n> >  \n> >  \trev_list.git_cmd = 1;\n> >  \trev_list.env = opt->env;\n> \n> This seems like a pretty safe and minimal backing-out for the -rc\n> period. We would still ship with \"--unsorted-input\" as an option (and\n> mentioned in the docs), though. If we're worried that it might be a dead\n> end and we don't want to support it, we could revert f45022dc2f\n> (connected: do not sort input revisions, 2021-08-09) entirely. That\n> carries a little more risk of accidentally breaking something during the\n> revert, but from my reading of the patch it should be pretty safe.\n> \n> I'd be curious to hear Patrick's thoughts on the whole thing.\n> \n> -Peff\n"},{"id":"441142","messageId":"YZJ8aDNVentnbalp@coredump.intra.peff.net","threadId":"56863","inReplyTo":"YZJ3hZkX9YjNBAdk@ncase","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-15T15:27:36Z","receivedAt":"2021-11-15T15:27:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 15, 2021 at 04:06:45PM +0100, Patrick Steinhardt wrote:\n\n> Sorry, but I'm currently completely loaded with work and thus didn't\n> find the capacity to have a deeper look yet and will probably not find\n> the time for a few more days. So the earliest I can have a look at this\n> is probably beginning next week.\n> \n> With that in mind, I'm happy to have this change reverted for now, as it\n> is...\n\nThanks for chiming in (and again, sorry for bringing this up so late in\nthe cycle).\n\n> > > We probably should revert this step as it can affect correctness in\n> > > a big way, but I wonder if the other steps in the same series, or\n> > > other topic that came later, rely on it.\n> > \n> > I looked them over, and I think this is pretty independent (with the\n> > exception of the refactoring of the no_walk/unsorted flags, but\n> > obviously that had to come first).\n> \n> .. completely independent of the other patches in this series and can be\n> reverted on its own. Only question is whether we also want to revert the\n> patch introducing this option in the first place given that it would end\n> up without a user afterwards.\n\nIt looks like Junio queued a revert of the whole patch in a7df4f52af\n(Revert \"connected: do not sort input revisions\", 2021-11-11), which is\non \"master\". So I think we should have a clean slate to look at this in\nthe next cycle.\n\n-Peff\n"},{"id":"441148","messageId":"YZKQXYdemZCNS/Bz@coredump.intra.peff.net","threadId":"56863","inReplyTo":"YY17rBFIdDl+H47I@coredump.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.34.0-rc2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-15T16:52:45Z","receivedAt":"2021-11-15T16:52:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2021 at 03:23:09PM -0500, Jeff King wrote:\n\n> Yes, I think that framing is right: it is making SLOP much worse. We\n> could similarly have had bogus timestamps in those commits which would\n> cause the same outcome. So in that sense it is nothing new. On the other\n> hand, I wonder how often it will cause extra traversal work (keeping in\n> mind that this commit traversal is just the first stage; after we find\n> the commits, then we talk all of their trees, which is the more\n> expensive part).\n> \n> For the case of adding new commits directly on top of another branch, I\n> think there would be no change. But any time you have to walk down to a\n> common fork point (e.g., imagine I made a new branch forked from an old\n> bit of history), we may fail to find that. I haven't quite constructed\n> an example, but I have a feeling we could end up walking over\n> arbitrarily long segments of history.\n\nI was playing around with this a bit more, and there is one subtlety in\nthe \"day-10\" snippet I showed that I hadn't noted before. It's important\nthe day-1 does not have any parents. If we used day-2 instead, then\nlimit_list() would insert its parent (day-1, in this case) into the\nqueue, without an UNINTERESTING flag (because it's the parent of\nsomething interesting).  And thus when we call still_interesting(), we\nwould never decrement the slop counter, because we know we are still\nwalking back to something potentially interesting.\n\nThis \"works\" because we put the new commit at the end of the list via\ncommit_list_insert_by_date(). That can be fooled, of course, because\nit's assuming the list is already in sorted order (which it isn't). So\nthere could be an \"old\" commit at the front, and we place the parent in\nfront of that, even though it's UNINTERESTING descendants are further\nback in the list.\n\nSo I do think we could walk an arbitrary string of history in this way,\nall the way down to the root, or to something else pointed to by a ref\ntip. Here's the example I came up with:\n\n-- >8 --\ngit init -q repo\ncd repo\n\ncommit_at() {\n\techo $1 >$1\n\tgit add .\n\tbase=1234567890\n\tunit=86400\n\ttimestamp=\"@$((base + $1 * unit)) +0000\"\n\tGIT_COMMITTER_DATE=$timestamp \\\n\tGIT_AUTHOR_DATE=$timestamp \\\n\tgit commit -qm \"commit at day $1\"\n}\n\n# imagine a bunch of base history\nfor i in $(seq 100); do\n\tcommit_at $i\ndone\ngit tag base\n\n# And then we have some older branches hanging around.\nfor i in $(seq 1 10); do\n\tgit checkout -b branch-$i base~$((60+$i))\ndone\n\n# But also a newer one; it's important that this refname\n# sort after the other ones, because that's what confuses\n# the sorting.\ngit branch new-branch\n\n# and then somebody pushes/fetches a branch based on an old part of history,\n# newer than our old branches, but older than our new one.\n#\n# We won't actually create the branch here, because we're simulating the state\n# before the ref is created, when we do the connectivity check.\nold_commit=$(git rev-parse base~50)\nnew_commit=$(echo foo | git commit-tree -p $old_commit HEAD^{tree})\n\n# and now here's the connectivity check we would do\ngit rev-list $new_commit --not --all >expect\ngit rev-list --unsorted-input $new_commit --not --all >actual\ndiff -u expect actual\n-- >8 --\n\nThat will report all of base~60..base~50 in the output, when it should\njust report the single new commit.\n\nI don't think any of this changes the plan for the 2.34 release (in\nfact, it makes me more confident that reverting this change was the\nright thing to do). I'm just recording my notes here for revisiting the\ntopic later.\n\nMy suspicion is that there's no easy way to make this work. We're\nviolating the assumption in still_interesting() that it can easily find\nthe lowest-date commit. We could drop that assumption for the unsorted\ncase, but then I think we'd be forced to walk all the way to the root\ncommits, which is even worse than sorting the tips.\n\nI suspect a better solution would be to make use of generation numbers\nfrom the commit graph if we have them. The --topo-order stuff already\ndoes this, and I kind of wonder if we could piggy-back on that.\n\n-Peff\n"}]}