{"thread":{"id":"49173","subject":"[ANNOUNCE] Git v2.19.0-rc0","startedAt":"2018-08-20T22:14:04Z","lastAt":"2018-09-02T18:54:03Z","messageCount":58,"participants":["Junio C Hamano","Stefan Beller","Jonathan Nieder","Derrick Stolee","Jeff King","brian m. carlson","Ævar Arnfjörð Bjarmason","Paul Smith","Duy Nguyen","Jacob Keller","Kaartic Sivaraam"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"356145","messageId":"xmqqwoskadpe.fsf@gitster-ct.c.googlers.com","threadId":"49173","inReplyTo":null,"subject":"[ANNOUNCE] Git v2.19.0-rc0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-20T22:13:49Z","receivedAt":"2018-08-20T22:14:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"An early preview release Git v2.19.0-rc0 is now available for\ntesting at the usual places. It is comprised of 707 non-merge\ncommits since v2.18.0, contributed by 60 people, 14 of which are\nnew 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.19.0-rc0' tag and the 'master' branch that the tag points at:\n\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.18.0 are as follows.\nWelcome to the Git development community!\n\n Aleksandr Makarov, Andrei Rybak, Chen Bin, Henning Schild,\n Isabella Stephens, Josh Steadmon, Jules Maselbas, Kana Natsuno,\n Marc Strapetz, Masaya Suzuki, Nicholas Guriev, Sebastian Kisela,\n Vladimir Parfinenko, and William Chargin.\n\nReturning contributors who helped this release are as follows.\nThanks for your continued support.\n\n Aaron Schrab, Ævar Arnfjörð Bjarmason, Alban Gruin, Alejandro\n R. Sedeño, Anthony Sottile, Antonio Ospite, Beat Bolli, Ben\n Peart, Brandon Williams, brian m. carlson, Christian Couder,\n Derrick Stolee, Elijah Newren, Eric Sunshine, Han-Wen Nienhuys,\n Jameson Miller, Jeff Hostetler, Jeff King, Johannes Schindelin,\n Johannes Sixt, Jonathan Nieder, Jonathan Tan, Junio C Hamano,\n Kim Gybels, Kirill Smelkov, Luis Marsano, Łukasz Stelmach,\n Luke Diamand, Martin Ågren, Max Kirillov, Michael Barabanov,\n Mike Hommey, Nguyễn Thái Ngọc Duy, Olga Telezhnaya,\n Phillip Wood, Prathamesh Chavan, Ramsay Jones, René Scharfe,\n Stefan Beller, SZEDER Gábor, Taylor Blau, Thomas Rast, Tobias\n Klauser, Todd Zullinger, Ville Skyttä, and Xiaolong Ye.\n\n----------------------------------------------------------------\n\nGit 2.19 Release Notes (draft)\n==============================\n\nUpdates since v2.18\n-------------------\n\nUI, Workflows & Features\n\n * \"git diff\" compares the index and the working tree. For paths\n added with intent-to-add bit, the command shows the full contents\n of them as added, but the paths themselves were not marked as new\n files. They are now shown as new by default.\n\n \"git apply\" learned the \"--intent-to-add\" option so that an\n otherwise working-tree-only application of a patch will add new\n paths to the index marked with the \"intent-to-add\" bit.\n\n * \"git grep\" learned the \"--column\" option that gives not just the\n line number but the column number of the hit.\n\n * The \"-l\" option in \"git branch -l\" is an unfortunate short-hand for\n \"--create-reflog\", but many users, both old and new, somehow expect\n it to be something else, perhaps \"--list\". This step warns when \"-l\"\n is used as a short-hand for \"--create-reflog\" and warns about the\n future repurposing of the it when it is used.\n\n * The userdiff pattern for .php has been updated.\n\n * The content-transfer-encoding of the message \"git send-email\" sends\n out by default was 8bit, which can cause trouble when there is an\n overlong line to bust RFC 5322/2822 limit. A new option 'auto' to\n automatically switch to quoted-printable when there is such a line\n in the payload has been introduced and is made the default.\n\n * \"git checkout\" and \"git worktree add\" learned to honor\n checkout.defaultRemote when auto-vivifying a local branch out of a\n remote tracking branch in a repository with multiple remotes that\n have tracking branches that share the same names.\n (merge 8d7b558bae ab/checkout-default-remote later to maint).\n\n * \"git grep\" learned the \"--only-matching\" option.\n\n * \"git rebase --rebase-merges\" mode now handles octopus merges as\n well.\n\n * Add a server-side knob to skip commits in exponential/fibbonacci\n stride in an attempt to cover wider swath of history with a smaller\n number of iterations, potentially accepting a larger packfile\n transfer, instead of going back one commit a time during common\n ancestor discovery during the \"git fetch\" transaction.\n (merge 42cc7485a2 jt/fetch-negotiator-skipping later to maint).\n\n * A new configuration variable core.usereplacerefs has been added,\n primarily to help server installations that want to ignore the\n replace mechanism altogether.\n\n * Teach \"git tag -s\" etc. a few configuration variables (gpg.format\n that can be set to \"openpgp\" or \"x509\", and gpg..program\n that is used to specify what program to use to deal with the format)\n to allow x.509 certs with CMS via \"gpgsm\" to be used instead of\n openpgp via \"gnupg\".\n\n * Many more strings are prepared for l10n.\n\n * \"git p4 submit\" learns to ask its own pre-submit hook if it should\n continue with submitting.\n\n * The test performed at the receiving end of \"git push\" to prevent\n bad objects from entering repository can be customized via\n receive.fsck.* configuration variables; we now have gained a\n counterpart to do the same on the \"git fetch\" side, with\n fetch.fsck.* configuration variables.\n\n * \"git pull --rebase=interactive\" learned \"i\" as a short-hand for\n \"interactive\".\n\n * \"git instaweb\" has been adjusted to run better with newer Apache on\n RedHat based distros.\n\n * \"git range-diff\" is a reimplementation of \"git tbdiff\" that lets us\n compare individual patches in two iterations of a topic.\n\n * The sideband code learned to optionally paint selected keywords at\n the beginning of incoming lines on the receiving end.\n\n\nPerformance, Internal Implementation, Development Support etc.\n\n * The bulk of \"git submodule foreach\" has been rewritten in C.\n\n * The in-core \"commit\" object had an all-purpose \"void *util\" field,\n which was tricky to use especially in library-ish part of the\n code. All of the existing uses of the field has been migrated to a\n more dedicated \"commit-slab\" mechanism and the field is eliminated.\n\n * A less often used command \"git show-index\" has been modernized.\n (merge fb3010c31f jk/show-index later to maint).\n\n * The conversion to pass \"the_repository\" and then \"a_repository\"\n throughout the object access API continues.\n\n * Continuing with the idea to programatically enumerate various\n pieces of data required for command line completion, teach the\n codebase to report the list of configuration variables\n subcommands care about to help complete them.\n\n * Separate \"rebase -p\" codepath out of \"rebase -i\" implementation to\n slim down the latter and make it easier to manage.\n\n * Make refspec parsing codepath more robust.\n\n * Some flaky tests have been fixed.\n\n * Continuing with the idea to programmatically enumerate various\n pieces of data required for command line completion, the codebase\n has been taught to enumerate options prefixed with \"--no-\" to\n negate them.\n\n * Build and test procedure for netrc credential helper (in contrib/)\n has been updated.\n\n * The conversion to pass \"the_repository\" and then \"a_repository\"\n throughout the object access API continues.\n\n * Remove unused function definitions and declarations from ewah\n bitmap subsystem.\n\n * Code preparation to make \"git p4\" closer to be usable with Python 3.\n\n * Tighten the API to make it harder to misuse in-tree .gitmodules\n file, even though it shares the same syntax with configuration\n files, to read random configuration items from it.\n\n * \"git fast-import\" has been updated to avoid attempting to create\n delta against a zero-byte-long string, which is pointless.\n\n * The codebase has been updated to compile cleanly with -pedantic\n option.\n (merge 2b647a05d7 bb/pedantic later to maint).\n\n * The character display width table has been updated to match the\n latest Unicode standard.\n (merge 570951eea2 bb/unicode-11-width later to maint).\n\n * test-lint now looks for broken use of \"VAR=VAL shell_func\" in test\n scripts.\n\n * Conversion from uchar[40] to struct object_id continues.\n\n * Recent \"security fix\" to pay attention to contents of \".gitmodules\"\n while accepting \"git push\" was a bit overly strict than necessary,\n which has been adjusted.\n\n * \"git fsck\" learns to make sure the optional commit-graph file is in\n a sane state.\n\n * \"git diff --color-moved\" feature has further been tweaked.\n\n * Code restructuring and a small fix to transport protocol v2 during\n fetching.\n\n * Parsing of -L[][,[]] parameters \"git blame\" and \"git log\"\n take has been tweaked.\n\n * lookup_commit_reference() and friends have been updated to find\n in-core object for a specific in-core repository instance.\n\n * Various glitches in the heuristics of merge-recursive strategy have\n been documented in new tests.\n\n * \"git fetch\" learned a new option \"--negotiation-tip\" to limit the\n set of commits it tells the other end as \"have\", to reduce wasted\n bandwidth and cycles, which would be helpful when the receiving\n repository has a lot of refs that have little to do with the\n history at the remote it is fetching from.\n\n * For a large tree, the index needs to hold many cache entries\n allocated on heap. These cache entries are now allocated out of a\n dedicated memory pool to amortize malloc(3) overhead.\n\n * Tests to cover various conflicting cases have been added for\n merge-recursive.\n\n * Tests to cover conflict cases that involve submodules have been\n added for merge-recursive.\n\n * Look for broken \"&&\" chains that are hidden in subshell, many of\n which have been found and corrected.\n\n * The singleton commit-graph in-core instance is made per in-core\n repository instance.\n\n * \"make DEVELOPER=1 DEVOPTS=pedantic\" allows developers to compile\n with -pedantic option, which may catch more problematic program\n constructs and potential bugs.\n\n * Preparatory code to later add json output for telemetry data has\n been added.\n\n * Update the way we use Coccinelle to find out-of-style code that\n need to be modernised.\n\n * It is too easy to misuse system API functions such as strcat();\n these selected functions are now forbidden in this codebase and\n will cause a compilation failure.\n\n * Add a script (in contrib/) to help users of VSCode work better with\n our codebase.\n\n * The Travis CI scripts were taught to ship back the test data from\n failed tests.\n (merge aea8879a6a sg/travis-retrieve-trash-upon-failure later to maint).\n\n * The parse-options machinery learned to refrain from enclosing\n placeholder string inside a \"\" pair automatically\n without PARSE_OPT_LITERAL_ARGHELP. Existing help text for option\n arguments that are not formatted correctly have been identified and\n fixed.\n (merge 5f0df44cd7 rs/parse-opt-lithelp later to maint).\n\n * Noiseword \"extern\" has been removed from function decls in the\n header files.\n\n * A few atoms like %(objecttype) and %(objectsize) in the format\n specifier of \"for-each-ref --format=\" can be filled without\n getting the full contents of the object, but just with the object\n header. These cases have been optimized by calling\n oid_object_info() API (instead of reading and inspecting the data).\n\n * The end result of documentation update has been made to be\n inspected more easily to help developers.\n\n * The API to iterate over all objects learned to optionally list\n objects in the order they appear in packfiles, which helps locality\n of access if the caller accesses these objects while as objects are\n enumerated.\n\n * Improve built-in facility to catch broken &&-chain in the tests.\n\n * The more library-ish parts of the codebase learned to work on the\n in-core index-state instance that is passed in by their callers,\n instead of always working on the singleton \"the_index\" instance.\n\n * A test prerequisite defined by various test scripts with slightly\n different semantics has been consolidated into a single copy and\n made into a lazily defined one.\n (merge 6ec633059a wc/make-funnynames-shared-lazy-prereq later to maint).\n\n * After a partial clone, repeated fetches from promisor remote would\n have accumulated many packfiles marked with .promisor bit without\n getting them coalesced into fewer packfiles, hurting performance.\n \"git repack\" now learned to repack them.\n\n\nFixes since v2.18\n-----------------\n\n * \"git remote update\" can take both a single remote nickname and a\n nickname for remote groups, and the completion script (in contrib/)\n has been taught about it.\n (merge 9cd4382ad5 ls/complete-remote-update-names later to maint).\n\n * \"git fetch --shallow-since=\" that specifies the cut-off\n point that is newer than the existing history used to end up\n grabbing the entire history. Such a request now errors out.\n (merge e34de73c56 nd/reject-empty-shallow-request later to maint).\n\n * Fix for 2.17-era regression around `core.safecrlf`.\n (merge 6cb09125be as/safecrlf-quiet-fix later to maint).\n\n * The recent addition of \"partial clone\" experimental feature kicked\n in when it shouldn't, namely, when there is no partial-clone filter\n defined even if extensions.partialclone is set.\n (merge cac1137dc4 jh/partial-clone later to maint).\n\n * \"git send-pack --signed\" (hence \"git push --signed\" over the http\n transport) did not read user ident from the config mechanism to\n determine whom to sign the push certificate as, which has been\n corrected.\n (merge d067d98887 ms/send-pack-honor-config later to maint).\n\n * \"git fetch-pack --all\" used to unnecessarily fail upon seeing an\n annotated tag that points at an object other than a commit.\n (merge c12c9df527 jk/fetch-all-peeled-fix later to maint).\n\n * When user edits the patch in \"git add -p\" and the user's editor is\n set to strip trailing whitespaces indiscriminately, an empty line\n that is unchanged in the patch would become completely empty\n (instead of a line with a sole SP on it). The code introduced in\n Git 2.17 timeframe failed to parse such a patch, but now it learned\n to notice the situation and cope with it.\n (merge f4d35a6b49 pw/add-p-recount later to maint).\n\n * The code to try seeing if a fetch is necessary in a submodule\n during a fetch with --recurse-submodules got confused when the path\n to the submodule was changed in the range of commits in the\n superproject, sometimes showing \"(null)\". This has been corrected.\n\n * \"git submodule\" did not correctly adjust core.worktree setting that\n indicates whether/where a submodule repository has its associated\n working tree across various state transitions, which has been\n corrected.\n (merge 984cd77ddb sb/submodule-core-worktree later to maint).\n\n * Bugfix for \"rebase -i\" corner case regression.\n (merge a9279c6785 pw/rebase-i-keep-reword-after-conflict later to maint).\n\n * Recently added \"--base\" option to \"git format-patch\" command did\n not correctly generate prereq patch ids.\n (merge 15b76c1fb3 xy/format-patch-prereq-patch-id-fix later to maint).\n\n * POSIX portability fix in Makefile to fix a glitch introduced a few\n releases ago.\n (merge 6600054e9b dj/runtime-prefix later to maint).\n\n * \"git filter-branch\" when used with the \"--state-branch\" option\n still attempted to rewrite the commits whose filtered result is\n known from the previous attempt (which is recorded on the state\n branch); the command has been corrected not to waste cycles doing\n so.\n (merge 709cfe848a mb/filter-branch-optim later to maint).\n\n * Clarify that setting core.ignoreCase to deviate from reality would\n not turn a case-incapable filesystem into a case-capable one.\n (merge 48294b512a ms/core-icase-doc later to maint).\n\n * \"fsck.skipList\" did not prevent a blob object listed there from\n being inspected for is contents (e.g. we recently started to\n inspect the contents of \".gitmodules\" for certain malicious\n patterns), which has been corrected.\n (merge fb16287719 rj/submodule-fsck-skip later to maint).\n\n * \"git checkout --recurse-submodules another-branch\" did not report\n in which submodule it failed to update the working tree, which\n resulted in an unhelpful error message.\n (merge ba95d4e4bd sb/submodule-move-head-error-msg later to maint).\n\n * \"git rebase\" behaved slightly differently depending on which one of\n the three backends gets used; this has been documented and an\n effort to make them more uniform has begun.\n (merge b00bf1c9a8 en/rebase-consistency later to maint).\n\n * The \"--ignore-case\" option of \"git for-each-ref\" (and its friends)\n did not work correctly, which has been fixed.\n (merge e674eb2528 jk/for-each-ref-icase later to maint).\n\n * \"git fetch\" failed to correctly validate the set of objects it\n received when making a shallow history deeper, which has been\n corrected.\n (merge cf1e7c0770 jt/connectivity-check-after-unshallow later to maint).\n\n * Partial clone support of \"git clone\" has been updated to correctly\n validate the objects it receives from the other side. The server\n side has been corrected to send objects that are directly\n requested, even if they may match the filtering criteria (e.g. when\n doing a \"lazy blob\" partial clone).\n (merge a7e67c11b8 jt/partial-clone-fsck-connectivity later to maint).\n\n * Handling of an empty range by \"git cherry-pick\" was inconsistent\n depending on how the range ended up to be empty, which has been\n corrected.\n (merge c5e358d073 jk/empty-pick-fix later to maint).\n\n * \"git reset --merge\" (hence \"git merge ---abort\") and \"git reset --hard\"\n had trouble working correctly in a sparsely checked out working\n tree after a conflict, which has been corrected.\n (merge b33fdfc34c mk/merge-in-sparse-checkout later to maint).\n\n * Correct a broken use of \"VAR=VAL shell_func\" in a test.\n (merge 650161a277 jc/t3404-one-shot-export-fix later to maint).\n\n * \"git rev-parse ':/substring'\" did not consider the history leading\n only to HEAD when looking for a commit with the given substring,\n when the HEAD is detached. This has been fixed.\n (merge 6b3351e799 wc/find-commit-with-pattern-on-detached-head later to maint).\n\n * Build doc update for Windows.\n (merge ede8d89bb1 nd/command-list later to maint).\n\n * core.commentchar is now honored when preparing the list of commits\n to replay in \"rebase -i\".\n\n * \"git pull --rebase\" on a corrupt HEAD caused a segfault. In\n general we substitute an empty tree object when running the in-core\n equivalent of the diff-index command, and the codepath has been\n corrected to do so as well to fix this issue.\n (merge 3506dc9445 jk/has-uncommitted-changes-fix later to maint).\n\n * httpd tests saw occasional breakage due to the way its access log\n gets inspected by the tests, which has been updated to make them\n less flaky.\n (merge e8b3b2e275 sg/httpd-test-unflake later to maint).\n\n * Tests to cover more D/F conflict cases have been added for\n merge-recursive.\n\n * \"git gc --auto\" opens file descriptors for the packfiles before\n spawning \"git repack/prune\", which would upset Windows that does\n not want a process to work on a file that is open by another\n process. The issue has been worked around.\n (merge 12e73a3ce4 kg/gc-auto-windows-workaround later to maint).\n\n * The recursive merge strategy did not properly ensure there was no\n change between HEAD and the index before performing its operation,\n which has been corrected.\n (merge 55f39cf755 en/dirty-merge-fixes later to maint).\n\n * \"git rebase\" started exporting GIT_DIR environment variable and\n exposing it to hook scripts when part of it got rewritten in C.\n Instead of matching the old scripted Porcelains' behaviour,\n compensate by also exporting GIT_WORK_TREE environment as well to\n lessen the damage. This can harm existing hooks that want to\n operate on different repository, but the current behaviour is\n already broken for them anyway.\n (merge ab5e67d751 bc/sequencer-export-work-tree-as-well later to maint).\n\n * \"git send-email\" when using in a batched mode that limits the\n number of messages sent in a single SMTP session lost the contents\n of the variable used to choose between tls/ssl, unable to send the\n second and later batches, which has been fixed.\n (merge 636f3d7ac5 jm/send-email-tls-auth-on-batch later to maint).\n\n * The lazy clone support had a few places where missing but promised\n objects were not correctly tolerated, which have been fixed.\n\n * One of the \"diff --color-moved\" mode \"dimmed_zebra\" that was named\n in an unusual way has been deprecated and replaced by\n \"dimmed-zebra\".\n (merge e3f2f5f9cd es/diff-color-moved-fix later to maint).\n\n * The wire-protocol v2 relies on the client to send \"ref prefixes\" to\n limit the bandwidth spent on the initial ref advertisement. \"git\n clone\" when learned to speak v2 forgot to do so, which has been\n corrected.\n (merge 402c47d939 bw/clone-ref-prefixes later to maint).\n\n * \"git diff --histogram\" had a bad memory usage pattern, which has\n been rearranged to reduce the peak usage.\n (merge 79cb2ebb92 sb/histogram-less-memory later to maint).\n\n * Code clean-up to use size_t/ssize_t when they are the right type.\n (merge 7726d360b5 jk/size-t later to maint).\n\n * The wire-protocol v2 relies on the client to send \"ref prefixes\" to\n limit the bandwidth spent on the initial ref advertisement. \"git\n fetch $remote branch:branch\" that asks tags that point into the\n history leading to the \"branch\" automatically followed sent to\n narrow prefix and broke the tag following, which has been fixed.\n (merge 2b554353a5 jt/tag-following-with-proto-v2-fix later to maint).\n\n * When the sparse checkout feature is in use, \"git cherry-pick\" and\n other mergy operations lost the skip_worktree bit when a path that\n is excluded from checkout requires content level merge, which is\n resolved as the same as the HEAD version, without materializing the\n merge result in the working tree, which made the path appear as\n deleted. This has been corrected by preserving the skip_worktree\n bit (and not materializing the file in the working tree).\n (merge 2b75fb601c en/merge-recursive-skip-fix later to maint).\n\n * The \"author-script\" file \"git rebase -i\" creates got broken when\n we started to move the command away from shell script, which is\n getting fixed now.\n (merge 5522bbac20 es/rebase-i-author-script-fix later to maint).\n\n * The automatic tree-matching in \"git merge -s subtree\" was broken 5\n years ago and nobody has noticed since then, which is now fixed.\n (merge 2ec4150713 jk/merge-subtree-heuristics later to maint).\n\n * \"git fetch $there refs/heads/s\" ought to fetch the tip of the\n branch 's', but when \"refs/heads/refs/heads/s\", i.e. a branch whose\n name is \"refs/heads/s\" exists at the same time, fetched that one\n instead by mistake. This has been corrected to honor the usual\n disambiguation rules for abbreviated refnames.\n (merge 60650a48c0 jt/refspec-dwim-precedence-fix later to maint).\n\n * Futureproofing a helper function that can easily be misused.\n (merge 65bb21e77e es/want-color-fd-defensive later to maint).\n\n * The http-backend (used for smart-http transport) used to slurp the\n whole input until EOF, without paying attention to CONTENT_LENGTH\n that is supplied in the environment and instead expecting the Web\n server to close the input stream. This has been fixed.\n (merge eebfe40962 mk/http-backend-content-length later to maint).\n\n * \"git merge --abort\" etc. did not clean things up properly when\n there were conflicted entries in the index in certain order that\n are involved in D/F conflicts. This has been corrected.\n (merge ad3762042a en/abort-df-conflict-fixes later to maint).\n\n * \"git diff --indent-heuristic\" had a bad corner case performance.\n (merge 301ef85401 sb/indent-heuristic-optim later to maint).\n\n * The \"--exec\" option to \"git rebase --rebase-merges\" placed the exec\n commands at wrong places, which has been corrected.\n\n * \"git verify-tag\" and \"git verify-commit\" have been taught to use\n the exit status of underlying \"gpg --verify\" to signal bad or\n untrusted signature they found.\n (merge 4e5dc9ca17 jc/gpg-status later to maint).\n\n * \"git mergetool\" stopped and gave an extra prompt to continue after\n the last path has been handled, which did not make much sense.\n (merge d651a54b8a ng/mergetool-lose-final-prompt later to maint).\n\n * Among the three codepaths we use O_APPEND to open a file for\n appending, one used for writing GIT_TRACE output requires O_APPEND\n implementation that behaves sensibly when multiple processes are\n writing to the same file. POSIX emulation used in the Windows port\n has been updated to improve in this area.\n (merge d641097589 js/mingw-o-append later to maint).\n\n * \"git pull --rebase -v\" in a repository with a submodule barfed as\n an intermediate process did not understand what \"-v(erbose)\" flag\n meant, which has been fixed.\n (merge e84c3cf3dc sb/pull-rebase-submodule later to maint).\n\n * Recent update to \"git config\" broke updating variable in a\n subsection, which has been corrected.\n (merge bff7df7a87 sb/config-write-fix later to maint).\n\n * When \"git rebase -i\" is told to squash two or more commits into\n one, it labeled the log message for each commit with its number.\n It correctly called the first one \"1st commit\", but the next one\n was \"commit #1\", which was off-by-one. This has been corrected.\n (merge dd2e36ebac pw/rebase-i-squash-number-fix later to maint).\n\n * \"git rebase -i\", when a 'merge ' insn in its todo list\n fails, segfaulted, which has been (minimally) corrected.\n (merge bc9238bb09 pw/rebase-i-merge-segv-fix later to maint).\n\n * \"git cherry-pick --quit\" failed to remove CHERRY_PICK_HEAD even\n though we won't be in a cherry-pick session after it returns, which\n has been corrected.\n (merge 3e7dd99208 nd/cherry-pick-quit-fix later to maint).\n\n * Code cleanup, docfix, build fix, etc.\n (merge aee9be2ebe sg/update-ref-stdin-cleanup later to maint).\n (merge 037714252f jc/clean-after-sanity-tests later to maint).\n (merge 5b26c3c941 en/merge-recursive-cleanup later to maint).\n (merge 0dcbc0392e bw/config-refer-to-gitsubmodules-doc later to maint).\n (merge bb4d000e87 bw/protocol-v2 later to maint).\n (merge 928f0ab4ba vs/typofixes later to maint).\n (merge d7f590be84 en/rebase-i-microfixes later to maint).\n (merge 81d395cc85 js/rebase-recreate-merge later to maint).\n (merge 51d1863168 tz/exclude-doc-smallfixes later to maint).\n (merge a9aa3c0927 ds/commit-graph later to maint).\n (merge 5cf8e06474 js/enhanced-version-info later to maint).\n (merge 6aaded5509 tb/config-default later to maint).\n (merge 022d2ac1f3 sb/blame-color later to maint).\n (merge 5a06a20e0c bp/test-drop-caches-for-windows later to maint).\n (merge dd61cc1c2e jk/ui-color-always-to-auto later to maint).\n (merge 1e83b9bfdd sb/trailers-docfix later to maint).\n (merge ab29f1b329 sg/fast-import-dump-refs-on-checkpoint-fix later to maint).\n (merge 6a8ad880f0 jn/subtree-test-fixes later to maint).\n (merge ffbd51cc60 nd/pack-objects-threading-doc later to maint).\n (merge e9dac7be60 es/mw-to-git-chain-fix later to maint).\n (merge fe583c6c7a rs/remote-mv-leakfix later to maint).\n (merge 69885ab015 en/t3031-title-fix later to maint).\n (merge 8578037bed nd/config-blame-sort later to maint).\n (merge 8ad169c4ba hn/config-in-code-comment later to maint).\n (merge b7446fcfdf ar/t4150-am-scissors-test-fix later to maint).\n (merge a8132410ee js/typofixes later to maint).\n (merge 388d0ff6e5 en/update-index-doc later to maint).\n (merge e05aa688dd jc/update-index-doc later to maint).\n (merge 10c600172c sg/t5310-empty-input-fix later to maint).\n (merge 5641eb9465 jh/partial-clone-doc later to maint).\n (merge 2711b1ad5e ab/submodule-relative-url-tests later to maint).\n\n----------------------------------------------------------------\n\nChanges since v2.18.0 are as follows:\n\nAaron Schrab (1):\n sequencer: use configured comment character\n\nAlban Gruin (4):\n rebase: introduce a dedicated backend for --preserve-merges\n rebase: strip unused code in git-rebase--preserve-merges.sh\n rebase: use the new git-rebase--preserve-merges.sh\n rebase: remove -p code from git-rebase--interactive.sh\n\nAlejandro R. Sedeño (1):\n Makefile: tweak sed invocation\n\nAleksandr Makarov (1):\n for-each-ref: consistently pass WM_IGNORECASE flag\n\nAndrei Rybak (2):\n Documentation: fix --color option formatting\n t4150: fix broken test for am --scissors\n\nAnthony Sottile (1):\n config.c: fix regression for core.safecrlf false\n\nAntonio Ospite (6):\n config: move config_from_gitmodules to submodule-config.c\n submodule-config: add helper function to get 'fetch' config from .gitmodules\n submodule-config: add helper to get 'update-clone' config from .gitmodules\n submodule-config: make 'config_from_gitmodules' private\n submodule-config: pass repository as argument to config_from_gitmodules\n submodule-config: reuse config_from_gitmodules in repo_read_gitmodules\n\nBeat Bolli (10):\n builtin/config: work around an unsized array forward declaration\n unicode: update the width tables to Unicode 11\n connect.h: avoid forward declaration of an enum\n refs/refs-internal.h: avoid forward declaration of an enum\n convert.c: replace \"\\e\" escapes with \"\\033\".\n sequencer.c: avoid empty statements at top level\n string-list.c: avoid conversion from void * to function pointer\n utf8.c: avoid char overflow\n Makefile: add a DEVOPTS flag to get pedantic compilation\n packfile: ensure that enum object_type is defined\n\nBen Peart (3):\n convert log_ref_write_fd() to use strbuf\n handle lower case drive letters on Windows\n t3507: add a testcase showing failure with sparse checkout\n\nBrandon Williams (15):\n commit: convert commit_graft_pos() to handle arbitrary repositories\n commit: convert register_commit_graft to handle arbitrary repositories\n commit: convert read_graft_file to handle arbitrary repositories\n test-pkt-line: add unpack-sideband subcommand\n docs: link to gitsubmodules\n upload-pack: implement ref-in-want\n upload-pack: test negotiation with changing repository\n fetch: refactor the population of peer ref OIDs\n fetch: refactor fetch_refs into two functions\n fetch: refactor to make function args narrower\n fetch-pack: put shallow info in output parameter\n fetch-pack: implement ref-in-want\n clone: send ref-prefixes when using protocol v2\n fetch-pack: mark die strings for translation\n pack-protocol: mention and point to docs for protocol v2\n\nChen Bin (1):\n git-p4: add the `p4-pre-submit` hook\n\nChristian Couder (1):\n t9104: kosherly remove remote refs\n\nDerrick Stolee (43):\n ref-filter: fix outdated comment on in_commit_list\n commit: add generation number to struct commit\n commit-graph: compute generation numbers\n commit: use generations in paint_down_to_common()\n commit-graph: always load commit-graph information\n ref-filter: use generation number for --contains\n commit: use generation numbers for in_merge_bases()\n commit: add short-circuit to paint_down_to_common()\n commit: use generation number in remove_redundant()\n merge: check config before loading commits\n commit-graph.txt: update design document\n commit-graph: fix UX issue when .lock file exists\n ewah/bitmap.c: delete unused 'bitmap_clear()'\n ewah/bitmap.c: delete unused 'bitmap_each_bit()'\n ewah_bitmap: delete unused 'ewah_and()'\n ewah_bitmap: delete unused 'ewah_and_not()'\n ewah_bitmap: delete unused 'ewah_not()'\n ewah_bitmap: delete unused 'ewah_or()'\n ewah_io: delete unused 'ewah_serialize()'\n t5318-commit-graph.sh: use core.commitGraph\n commit-graph: UNLEAK before die()\n commit-graph: fix GRAPH_MIN_SIZE\n commit-graph: parse commit from chosen graph\n commit: force commit to parse from object database\n commit-graph: load a root tree from specific graph\n commit-graph: add 'verify' subcommand\n commit-graph: verify catches corrupt signature\n commit-graph: verify required chunks are present\n commit-graph: verify corrupt OID fanout and lookup\n commit-graph: verify objects exist\n commit-graph: verify root tree OIDs\n commit-graph: verify parent list\n commit-graph: verify generation number\n commit-graph: verify commit date\n commit-graph: test for corrupted octopus edge\n commit-graph: verify contents match checksum\n fsck: verify commit-graph\n commit-graph: use string-list API for input\n commit-graph: add '--reachable' option\n gc: automatically write commit-graph files\n commit-graph: update design document\n commit-graph: fix documentation inconsistencies\n coccinelle: update commit.cocci\n\nElijah Newren (63):\n t6036, t6042: use test_create_repo to keep tests independent\n t6036, t6042: use test_line_count instead of wc -l\n t6036, t6042: prefer test_path_is_file, test_path_is_missing\n t6036, t6042: prefer test_cmp to sequences of test\n t6036: prefer test_when_finished to manual cleanup in following test\n merge-recursive: fix miscellaneous grammar error in comment\n merge-recursive: fix numerous argument alignment issues\n merge-recursive: align labels with their respective code blocks\n merge-recursive: clarify the rename_dir/RENAME_DIR meaning\n merge-recursive: rename conflict_rename_*() family of functions\n merge-recursive: add pointer about unduly complex looking code\n git-rebase.txt: document incompatible options\n git-rebase.sh: update help messages a bit\n t3422: new testcases for checking when incompatible options passed\n git-rebase: error out when incompatible options passed\n git-rebase.txt: address confusion between --no-ff vs --force-rebase\n directory-rename-detection.txt: technical docs on abilities and limitations\n git-rebase.txt: document behavioral differences between modes\n t3401: add directory rename testcases for rebase and am\n git-rebase: make --allow-empty-message the default\n t3418: add testcase showing problems with rebase -i and strategy options\n Fix use of strategy options with interactive rebases\n git-rebase--merge: modernize \"git-$cmd\" to \"git $cmd\"\n apply: fix grammar error in comment\n t5407: fix test to cover intended arguments\n read-cache.c: move index_has_changes() from merge.c\n index_has_changes(): avoid assuming operating on the_index\n t6044: verify that merges expected to abort actually abort\n t6036: add a failed conflict detection case with symlink modify/modify\n t6036: add a failed conflict detection case with symlink add/add\n t6036: add a failed conflict detection case with submodule modify/modify\n t6036: add a failed conflict detection case with submodule add/add\n t6036: add a failed conflict detection case with conflicting types\n t6042: add testcase covering rename/add/delete conflict type\n t6042: add testcase covering rename/rename(2to1)/delete/delete conflict\n t6042: add testcase covering long chains of rename conflicts\n t6036: add lots of detail for directory/file conflicts in recursive case\n t6036: add a failed conflict detection case: regular files, different modes\n t6044: add a testcase for index matching head, when head doesn't match HEAD\n merge-recursive: make sure when we say we abort that we actually abort\n merge-recursive: fix assumption that head tree being merged is HEAD\n t6044: add more testcases with staged changes before a merge is invoked\n merge-recursive: enforce rule that index matches head before merging\n merge: fix misleading pre-merge check documentation\n t7405: add a file/submodule conflict\n t7405: add a directory/submodule conflict\n t7405: verify 'merge --abort' works after submodule/path conflicts\n merge-recursive: preserve skip_worktree bit when necessary\n t1015: demonstrate directory/file conflict recovery failures\n read-cache: fix directory/file conflict handling in read_index_unmerged()\n t3031: update test description to mention desired behavior\n t7406: fix call that was failing for the wrong reason\n t7406: simplify by using diff --name-only instead of diff --raw\n t7406: avoid having git commands upstream of a pipe\n t7406: prefer test_* helper functions to test -[feds]\n t7406: avoid using test_must_fail for commands other than git\n git-update-index.txt: reword possibly confusing example\n Add missing includes and forward declarations\n alloc: make allocate_alloc_state and clear_alloc_state more consistent\n Move definition of enum branch_track from cache.h to branch.h\n urlmatch.h: fix include guard\n compat/precompose_utf8.h: use more common include guard style\n Remove forward declaration of an enum\n\nEric Sunshine (53):\n t: use test_might_fail() instead of manipulating exit code manually\n t: use test_write_lines() instead of series of 'echo' commands\n t: use sane_unset() rather than 'unset' with broken &&-chain\n t: drop unnecessary terminating semicolon in subshell\n t/lib-submodule-update: fix \"absorbing\" test\n t5405: use test_must_fail() instead of checking exit code manually\n t5406: use write_script() instead of birthing shell script manually\n t5505: modernize and simplify hard-to-digest test\n t6036: fix broken \"merge fails but has appropriate contents\" tests\n t7201: drop pointless \"exit 0\" at end of subshell\n t7400: fix broken \"submodule add/reconfigure --force\" test\n t7810: use test_expect_code() instead of hand-rolled comparison\n t9001: fix broken \"invoke hook\" test\n t9814: simplify convoluted check that command correctly errors out\n t0000-t0999: fix broken &&-chains\n t1000-t1999: fix broken &&-chains\n t2000-t2999: fix broken &&-chains\n t3000-t3999: fix broken &&-chains\n t3030: fix broken &&-chains\n t4000-t4999: fix broken &&-chains\n t5000-t5999: fix broken &&-chains\n t6000-t6999: fix broken &&-chains\n t7000-t7999: fix broken &&-chains\n t9000-t9999: fix broken &&-chains\n t9119: fix broken &&-chains\n t6046/t9833: fix use of \"VAR=VAL cmd\" with a shell function\n t/check-non-portable-shell: stop being so polite\n t/check-non-portable-shell: make error messages more compact\n t/check-non-portable-shell: detect \"FOO=bar shell_func\"\n t/test-lib: teach --chain-lint to detect broken &&-chains in subshells\n t/Makefile: add machinery to check correctness of chainlint.sed\n t/chainlint: add chainlint \"basic\" test cases\n t/chainlint: add chainlint \"whitespace\" test cases\n t/chainlint: add chainlint \"one-liner\" test cases\n t/chainlint: add chainlint \"nested subshell\" test cases\n t/chainlint: add chainlint \"loop\" and \"conditional\" test cases\n t/chainlint: add chainlint \"cuddled\" test cases\n t/chainlint: add chainlint \"complex\" test cases\n t/chainlint: add chainlint \"specialized\" test cases\n diff: --color-moved: rename \"dimmed_zebra\" to \"dimmed-zebra\"\n mw-to-git/t9360: fix broken &&-chain\n t/chainlint.sed: drop extra spaces from regex character class\n sequencer: fix \"rebase -i --root\" corrupting author header\n sequencer: fix \"rebase -i --root\" corrupting author header timezone\n sequencer: fix \"rebase -i --root\" corrupting author header timestamp\n sequencer: don't die() on bogus user-edited timestamp\n color: protect against out-of-bounds reads and writes\n chainlint: match arbitrary here-docs tags rather than hard-coded names\n chainlint: match 'quoted' here-doc tags\n chainlint: recognize multi-line $(...) when command cuddled with \"$(\"\n chainlint: let here-doc and multi-line string commence on same line\n chainlint: recognize multi-line quoted strings more robustly\n chainlint: add test of pathological case which triggered false positive\n\nHan-Wen Nienhuys (2):\n config: document git config getter return value\n sideband: highlight keywords in remote sideband output\n\nHenning Schild (9):\n builtin/receive-pack: use check_signature from gpg-interface\n gpg-interface: make parse_gpg_output static and remove from interface header\n gpg-interface: add new config to select how to sign a commit\n t/t7510: check the validation of the new config gpg.format\n gpg-interface: introduce an abstraction for multiple gpg formats\n gpg-interface: do not hardcode the key string len anymore\n gpg-interface: introduce new config to select per gpg format program\n gpg-interface: introduce new signature format \"x509\" using gpgsm\n gpg-interface t: extend the existing GPG tests with GPGSM\n\nIsabella Stephens (2):\n blame: prevent error if range ends past end of file\n log: prevent error if line range ends past end of file\n\nJameson Miller (8):\n read-cache: teach refresh_cache_entry to take istate\n read-cache: teach make_cache_entry to take object_id\n block alloc: add lifecycle APIs for cache_entry structs\n mem-pool: only search head block for available space\n mem-pool: add life cycle management functions\n mem-pool: fill out functionality\n block alloc: allocate cache entries from mem_pool\n block alloc: add validations around cache_entry lifecyle\n\nJeff Hostetler (1):\n json_writer: new routines to create JSON data\n\nJeff King (48):\n make show-index a builtin\n show-index: update documentation for index v2\n fetch-pack: don't try to fetch peel values with --all\n ewah: drop ewah_deserialize function\n ewah: drop ewah_serialize_native function\n t3200: unset core.logallrefupdates when testing reflog creation\n t: switch \"branch -l\" to \"branch --create-reflog\"\n branch: deprecate \"-l\" option\n config: turn die_on_error into caller-facing enum\n config: add CONFIG_ERROR_SILENT handler\n config: add options parameter to git_config_from_mem\n fsck: silence stderr when parsing .gitmodules\n t6300: add a test for --ignore-case\n ref-filter: avoid backend filtering with --ignore-case\n t5500: prettify non-commit tag tests\n sequencer: handle empty-set cases consistently\n sequencer: don't say BUG on bogus input\n has_uncommitted_changes(): fall back to empty tree\n fsck: split \".gitmodules too large\" error from parse failure\n fsck: downgrade gitmodulesParse default to \"info\"\n blame: prefer xsnprintf to strcpy for colors\n check_replace_refs: fix outdated comment\n check_replace_refs: rename to read_replace_refs\n add core.usereplacerefs config option\n reencode_string: use st_add/st_mult helpers\n reencode_string: use size_t for string lengths\n strbuf: use size_t for length in intermediate variables\n strbuf_readlink: use ssize_t\n pass st.st_size as hint for strbuf_readlink()\n strbuf_humanise: use unsigned variables\n automatically ban strcpy()\n banned.h: mark strcat() as banned\n banned.h: mark sprintf() as banned\n banned.h: mark strncpy() as banned\n score_trees(): fix iteration over trees with missing entries\n add a script to diff rendered documentation\n t5552: suppress upload-pack trace output\n for_each_*_object: store flag definitions in a single location\n for_each_*_object: take flag arguments as enum\n for_each_*_object: give more comprehensive docstrings\n for_each_packed_object: support iterating in pack-order\n t1006: test cat-file --batch-all-objects with duplicates\n cat-file: rename batch_{loose,packed}_object callbacks\n cat-file: support \"unordered\" output for --batch-all-objects\n cat-file: use oidset check-and-insert\n cat-file: split batch \"buf\" into two variables\n cat-file: use a single strbuf for all output\n for_each_*_object: move declarations to object-store.h\n\nJohannes Schindelin (41):\n Makefile: fix the \"built from commit\" code\n merge: allow reading the merge commit message from a file\n rebase --rebase-merges: add support for octopus merges\n rebase --rebase-merges: adjust man page for octopus support\n vcbuild/README: update to accommodate for missing common-cmds.h\n t7406: avoid failures solely due to timing issues\n contrib: add a script to initialize VS Code configuration\n vscode: hard-code a couple defines\n cache.h: extract enum declaration from inside a struct declaration\n mingw: define WIN32 explicitly\n vscode: only overwrite C/C++ settings\n vscode: wrap commit messages at column 72 by default\n vscode: use 8-space tabs, no trailing ws, etc for Git's source code\n vscode: add a dictionary for cSpell\n vscode: let cSpell work on commit messages, too\n pull --rebase=: allow single-letter abbreviations for the type\n t3430: demonstrate what -r, --autosquash & --exec should do\n git-compat-util.h: fix typo\n remote-curl: remove spurious period\n rebase --exec: make it work with --rebase-merges\n linear-assignment: a function to solve least-cost assignment problems\n Introduce `range-diff` to compare iterations of a topic branch\n range-diff: first rudimentary implementation\n range-diff: improve the order of the shown commits\n range-diff: also show the diff between patches\n range-diff: right-trim commit messages\n range-diff: indent the diffs just like tbdiff\n range-diff: suppress the diff headers\n range-diff: adjust the output of the commit pairs\n range-diff: do not show \"function names\" in hunk headers\n range-diff: use color for the commit pairs\n color: add the meta color GIT_COLOR_REVERSE\n diff: add an internal option to dual-color diffs of diffs\n range-diff: offer to dual-color the diffs\n range-diff --dual-color: skip white-space warnings\n range-diff: populate the man page\n completion: support `git range-diff`\n range-diff: left-pad patch numbers\n range-diff: make --dual-color the default mode\n range-diff: use dim/bold cues to improve dual color mode\n chainlint: fix for core.autocrlf=true\n\nJohannes Sixt (1):\n mingw: enable atomic O_APPEND\n\nJonathan Nieder (11):\n object: add repository argument to grow_object_hash\n object: move grafts to object parser\n commit: add repository argument to commit_graft_pos\n commit: add repository argument to register_commit_graft\n commit: add repository argument to read_graft_file\n commit: add repository argument to prepare_commit_graft\n commit: add repository argument to lookup_commit_graft\n subtree test: add missing && to &&-chain\n subtree test: simplify preparation of expected results\n doc hash-function-transition: pick SHA-256 as NewHash\n partial-clone: render design doc using asciidoc\n\nJonathan Tan (28):\n list-objects: check if filter is NULL before using\n fetch-pack: split up everything_local()\n fetch-pack: clear marks before re-marking\n fetch-pack: directly end negotiation if ACK ready\n fetch-pack: use ref adv. to prune \"have\" sent\n fetch-pack: make negotiation-related vars local\n fetch-pack: move common check and marking together\n fetch-pack: introduce negotiator API\n pack-bitmap: remove bitmap_git global variable\n pack-bitmap: add free function\n fetch-pack: write shallow, then check connectivity\n fetch-pack: support negotiation tip whitelist\n upload-pack: send refs' objects despite \"filter\"\n clone: check connectivity even if clone is partial\n revision: tolerate promised targets of tags\n tag: don't warn if target is missing but promised\n negotiator/skipping: skip commits during fetch\n commit-graph: refactor preparing commit graph\n object-store: add missing include\n commit-graph: add missing forward declaration\n commit-graph: add free_commit_graph\n commit-graph: store graph in struct object_store\n commit-graph: add repo arg to graph readers\n t5702: test fetch with multiple refspecs at a time\n fetch: send \"refs/tags/\" prefix upon CLI refspecs\n fetch-pack: unify ref in and out param\n repack: refactor setup of pack-objects cmd\n repack: repack promisor objects if -a or -A is set\n\nJosh Steadmon (1):\n protocol-v2 doc: put HTTP headers after request\n\nJules Maselbas (1):\n send-email: fix tls AUTH when sending batch\n\nJunio C Hamano (18):\n tests: clean after SANITY tests\n ewah: delete unused 'rlwit_discharge_empty()'\n Prepare to start 2.19 cycle\n First batch for 2.19 cycle\n Second batch for 2.19 cycle\n fixup! connect.h: avoid forward declaration of an enum\n fixup! refs/refs-internal.h: avoid forward declaration of an enum\n t3404: fix use of \"VAR=VAL cmd\" with a shell function\n Third batch for 2.19 cycle\n Fourth batch for 2.19 cycle\n remote: make refspec follow the same disambiguation rule as local refs\n Fifth batch for 2.19 cycle\n update-index: there no longer is `apply --index-info`\n gpg-interface: propagate exit status from gpg back to the callers\n Sixth batch for 2.19 cycle\n Seventh batch for 2.19 cycle\n sideband: do not read beyond the end of input\n Git 2.19-rc0\n\nKana Natsuno (2):\n t4018: add missing test cases for PHP\n userdiff: support new keywords in PHP hunk header\n\nKim Gybels (1):\n gc --auto: release pack files before auto packing\n\nKirill Smelkov (1):\n fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits\n\nLuis Marsano (2):\n git-credential-netrc: use in-tree Git.pm for tests\n git-credential-netrc: fix exit status when tests fail\n\nLuke Diamand (6):\n git-p4: python3: replace <> with !=\n git-p4: python3: replace dict.has_key(k) with \"k in dict\"\n git-p4: python3: remove backticks\n git-p4: python3: basestring workaround\n git-p4: python3: use print() function\n git-p4: python3: fix octal constants\n\nMarc Strapetz (1):\n Documentation: declare \"core.ignoreCase\" as internal variable\n\nMartin Ågren (1):\n refspec: initalize `refspec_item` in `valid_fetch_refspec()`\n\nMasaya Suzuki (2):\n builtin/send-pack: populate the default configs\n doc: fix want-capability separator\n\nMax Kirillov (4):\n http-backend: cleanup writing to child process\n http-backend: respect CONTENT_LENGTH as specified by rfc3875\n unpack-trees: do not fail reset because of unmerged skipped entry\n http-backend: respect CONTENT_LENGTH for receive-pack\n\nMichael Barabanov (1):\n filter-branch: skip commits present on --state-branch\n\nMike Hommey (1):\n fast-import: do not call diff_delta() with empty buffer\n\nNguyễn Thái Ngọc Duy (98):\n commit-slab.h: code split\n commit-slab: support shared commit-slab\n blame: use commit-slab for blame suspects instead of commit->util\n describe: use commit-slab for commit names instead of commit->util\n shallow.c: use commit-slab for commit depth instead of commit->util\n sequencer.c: use commit-slab to mark seen commits\n sequencer.c: use commit-slab to associate todo items to commits\n revision.c: use commit-slab for show_source\n bisect.c: use commit-slab for commit weight instead of commit->util\n name-rev: use commit-slab for rev-name instead of commit->util\n show-branch: use commit-slab for commit-name instead of commit->util\n show-branch: note about its object flags usage\n log: use commit-slab in prepare_bases() instead of commit->util\n merge: use commit-slab in merge remote desc instead of commit->util\n commit.h: delete 'util' field in struct commit\n diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree\n diff: turn --ita-invisible-in-index on by default\n t2203: add a test about \"diff HEAD\" case\n apply: add --intent-to-add\n parse-options: option to let --git-completion-helper show negative form\n completion: suppress some -no- options\n Add and use generic name->id mapping code for color slot parsing\n grep: keep all colors in an array\n fsck: factor out msg_id_info[] lazy initialization code\n help: add --config to list all available config\n fsck: produce camelCase config key names\n advice: keep config name in camelCase in advice_config[]\n am: move advice.amWorkDir parsing back to advice.c\n completion: drop the hard coded list of config vars\n completion: keep other config var completion in camelCase\n completion: support case-insensitive config vars\n log-tree: allow to customize 'grafted' color\n completion: complete general config vars in two steps\n upload-pack: reject shallow requests that would return nothing\n completion: collapse extra --no-.. options\n Update messages in preparation for i18n\n archive-tar.c: mark more strings for translation\n archive-zip.c: mark more strings for translation\n builtin/config.c: mark more strings for translation\n builtin/grep.c: mark strings for translation\n builtin/pack-objects.c: mark more strings for translation\n builtin/replace.c: mark more strings for translation\n commit-graph.c: mark more strings for translation\n config.c: mark more strings for translation\n connect.c: mark more strings for translation\n convert.c: mark more strings for translation\n dir.c: mark more strings for translation\n environment.c: mark more strings for translation\n exec-cmd.c: mark more strings for translation\n object.c: mark more strings for translation\n pkt-line.c: mark more strings for translation\n refs.c: mark more strings for translation\n refspec.c: mark more strings for translation\n replace-object.c: mark more strings for translation\n sequencer.c: mark more strings for translation\n sha1-file.c: mark more strings for translation\n transport.c: mark more strings for translation\n transport-helper.c: mark more strings for translation\n pack-objects: document about thread synchronization\n apply.h: drop extern on func declaration\n attr.h: drop extern from function declaration\n blame.h: drop extern on func declaration\n cache-tree.h: drop extern from function declaration\n convert.h: drop 'extern' from function declaration\n diffcore.h: drop extern from function declaration\n diff.h: remove extern from function declaration\n line-range.h: drop extern from function declaration\n rerere.h: drop extern from function declaration\n repository.h: drop extern from function declaration\n revision.h: drop extern from function declaration\n submodule.h: drop extern from function declaration\n config.txt: reorder blame stuff to keep config keys sorted\n Makefile: add missing dependency for command-list.h\n diff.c: move read_index() code back to the caller\n cache-tree: wrap the_index based wrappers with #ifdef\n attr: remove an implicit dependency on the_index\n convert.c: remove an implicit dependency on the_index\n dir.c: remove an implicit dependency on the_index in pathspec code\n preload-index.c: use the right index instead of the_index\n ls-files: correct index argument to get_convert_attr_ascii()\n unpack-trees: remove 'extern' on function declaration\n unpack-trees: add a note about path invalidation\n unpack-trees: don't shadow global var the_index\n unpack-trees: convert clear_ce_flags* to avoid the_index\n unpack-trees: avoid the_index in verify_absent()\n pathspec.c: use the right index instead of the_index\n submodule.c: use the right index instead of the_index\n entry.c: use the right index instead of the_index\n attr: remove index from git_attr_set_direction()\n grep: use the right index instead of the_index\n archive.c: avoid access to the_index\n archive-*.c: use the right repository\n resolve-undo.c: use the right index instead of the_index\n apply.c: pass struct apply_state to more functions\n apply.c: make init_apply_state() take a struct repository\n apply.c: remove implicit dependency on the_index\n blame.c: remove implicit dependency on the_index\n cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD\n\nNicholas Guriev (1):\n mergetool: don't suggest to continue after last file\n\nOlga Telezhnaya (5):\n ref-filter: add info_source to valid_atom\n ref-filter: fill empty fields with empty values\n ref-filter: initialize eaten variable\n ref-filter: merge get_obj and get_object\n ref-filter: use oid_object_info() to get object\n\nPhillip Wood (5):\n add -p: fix counting empty context lines in edited patches\n sequencer: do not squash 'reword' commits when we hit conflicts\n rebase -i: fix numbering in squash message\n t3430: add conflicting commit\n rebase -i: fix SIGSEGV when 'merge ' fails\n\nPrathamesh Chavan (4):\n submodule foreach: correct '$path' in nested submodules from a subdirectory\n submodule foreach: document '$sm_path' instead of '$path'\n submodule foreach: document variable '$displaypath'\n submodule: port submodule subcommand 'foreach' from shell to C\n\nRamsay Jones (3):\n fsck: check skiplist for object in fsck_blob()\n t6036: fix broken && chain in sub-shell\n t5562: avoid non-portable \"export FOO=bar\" construct\n\nRené Scharfe (7):\n remote: clear string_list after use in mv()\n add, update-index: fix --chmod argument help\n difftool: remove angular brackets from argument help\n pack-objects: specify --index-version argument help explicitly\n send-pack: specify --force-with-lease argument help explicitly\n shortlog: correct option help for -w\n parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP\n\nSZEDER Gábor (19):\n update-ref --stdin: use skip_prefix()\n t7510-signed-commit: use 'test_must_fail'\n tests: make forging GPG signed commits and tags more robust\n t5541: clean up truncating access log\n t/lib-httpd: add the strip_access_log() helper function\n t/lib-httpd: avoid occasional failures when checking access.log\n t5608: fix broken &&-chain\n t9300: wait for background fast-import process to die after killing it\n travis-ci: run Coccinelle static analysis with two parallel jobs\n travis-ci: fail if Coccinelle static analysis found something to transform\n coccinelle: mark the 'coccicheck' make target as .PHONY\n coccinelle: use $(addsuffix) in 'coccicheck' make target\n coccinelle: exclude sha1dc source files from static analysis\n coccinelle: put sane filenames into output patches\n coccinelle: extract dedicated make target to clean Coccinelle's results\n travis-ci: include the trash directories of failed tests in the trace log\n t5318: use 'test_cmp_bin' to compare commit-graph files\n t5318: avoid unnecessary command substitutions\n t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test\n\nSebastian Kisela (2):\n git-instaweb: support Fedora/Red Hat apache module path\n git-instaweb: fix apache2 config with apache >= 2.4\n\nStefan Beller (87):\n repository: introduce parsed objects field\n object: add repository argument to create_object\n alloc: add repository argument to alloc_blob_node\n alloc: add repository argument to alloc_tree_node\n alloc: add repository argument to alloc_commit_node\n alloc: add repository argument to alloc_tag_node\n alloc: add repository argument to alloc_object_node\n alloc: add repository argument to alloc_report\n alloc: add repository argument to alloc_commit_index\n object: allow grow_object_hash to handle arbitrary repositories\n object: allow create_object to handle arbitrary repositories\n alloc: allow arbitrary repositories for alloc functions\n object-store: move object access functions to object-store.h\n shallow: add repository argument to set_alternate_shallow_file\n shallow: add repository argument to register_shallow\n shallow: add repository argument to check_shallow_file_for_update\n shallow: add repository argument to is_repository_shallow\n cache: convert get_graft_file to handle arbitrary repositories\n path.c: migrate global git_path_* to take a repository argument\n shallow: migrate shallow information into the object parser\n commit: allow prepare_commit_graft to handle arbitrary repositories\n commit: allow lookup_commit_graft to handle arbitrary repositories\n refs/packed-backend.c: close fd of empty file\n submodule--helper: plug mem leak in print_default_remote\n sequencer.c: plug leaks in do_pick_commit\n submodule: fix NULL correctness in renamed broken submodules\n t5526: test recursive submodules when fetching moved submodules\n submodule: unset core.worktree if no working tree is present\n submodule: ensure core.worktree is set after update\n submodule deinit: unset core.worktree\n submodule.c: report the submodule that an error occurs in\n sequencer.c: plug mem leak in git_sequencer_config\n .mailmap: merge different spellings of names\n object: add repository argument to parse_object\n object: add repository argument to lookup_object\n object: add repository argument to parse_object_buffer\n object: add repository argument to object_as_type\n blob: add repository argument to lookup_blob\n tree: add repository argument to lookup_tree\n commit: add repository argument to lookup_commit_reference_gently\n commit: add repository argument to lookup_commit_reference\n commit: add repository argument to lookup_commit\n commit: add repository argument to parse_commit_buffer\n commit: add repository argument to set_commit_buffer\n commit: add repository argument to get_cached_commit_buffer\n tag: add repository argument to lookup_tag\n tag: add repository argument to parse_tag_buffer\n tag: add repository argument to deref_tag\n object: allow object_as_type to handle arbitrary repositories\n object: allow lookup_object to handle arbitrary repositories\n blob: allow lookup_blob to handle arbitrary repositories\n tree: allow lookup_tree to handle arbitrary repositories\n commit: allow lookup_commit to handle arbitrary repositories\n tag: allow lookup_tag to handle arbitrary repositories\n tag: allow parse_tag_buffer to handle arbitrary repositories\n commit.c: allow parse_commit_buffer to handle arbitrary repositories\n commit-slabs: remove realloc counter outside of slab struct\n commit.c: migrate the commit buffer to the parsed object store\n commit.c: allow set_commit_buffer to handle arbitrary repositories\n commit.c: allow get_cached_commit_buffer to handle arbitrary repositories\n object.c: allow parse_object_buffer to handle arbitrary repositories\n object.c: allow parse_object to handle arbitrary repositories\n tag.c: allow deref_tag to handle arbitrary repositories\n commit.c: allow lookup_commit_reference_gently to handle arbitrary repositories\n commit.c: allow lookup_commit_reference to handle arbitrary repositories\n xdiff/xdiff.h: remove unused flags\n xdiff/xdiffi.c: remove unneeded function declarations\n t4015: avoid git as a pipe input\n diff.c: do not pass diff options as keydata to hashmap\n diff.c: adjust hash function signature to match hashmap expectation\n diff.c: add a blocks mode for moved code detection\n diff.c: decouple white space treatment from move detection algorithm\n diff.c: factor advance_or_nullify out of mark_color_as_moved\n diff.c: add white space mode to move detection that allows indent changes\n diff.c: offer config option to control ws handling in move detection\n xdiff/xhistogram: pass arguments directly to fall_back_to_classic_diff\n xdiff/xhistogram: factor out memory cleanup into free_index()\n xdiff/xhistogram: move index allocation into find_lcs\n Documentation/git-interpret-trailers: explain possible values\n xdiff/histogram: remove tail recursion\n t1300: document current behavior of setting options\n xdiff: reduce indent heuristic overhead\n config: fix case sensitive subsection names on writing\n git-config: document accidental multi-line setting in deprecated syntax\n git-submodule.sh: accept verbose flag in cmd_update to be non-quiet\n t7410: update to new style\n builtin/submodule--helper: remove stray new line\n\nTaylor Blau (9):\n Documentation/config.txt: camel-case lineNumber for consistency\n grep.c: expose {,inverted} match column in match_line()\n grep.[ch]: extend grep_opt to allow showing matched column\n grep.c: display column number of first match\n builtin/grep.c: add '--column' option to 'git-grep(1)'\n grep.c: add configuration variables to show matched option\n contrib/git-jump/git-jump: jump to exact location\n grep.c: extract show_line_header()\n grep.c: teach 'git grep --only-matching'\n\nThomas Rast (1):\n range-diff: add tests\n\nTobias Klauser (1):\n git-rebase--preserve-merges: fix formatting of todo help message\n\nTodd Zullinger (4):\n git-credential-netrc: minor whitespace cleanup in test script\n git-credential-netrc: make \"all\" default target of Makefile\n gitignore.txt: clarify default core.excludesfile path\n dir.c: fix typos in core.excludesfile comment\n\nVille Skyttä (1):\n Documentation: spelling and grammar fixes\n\nVladimir Parfinenko (1):\n rebase: fix documentation formatting\n\nWilliam Chargin (2):\n sha1-name.c: for \":/\", find detached HEAD commits\n t: factor out FUNNYNAMES as shared lazy prereq\n\nXiaolong Ye (1):\n format-patch: clear UNINTERESTING flag before prepare_bases\n\nbrian m. carlson (21):\n send-email: add an auto option for transfer encoding\n send-email: accept long lines with suitable transfer encoding\n send-email: automatically determine transfer-encoding\n docs: correct RFC specifying email line length\n sequencer: pass absolute GIT_WORK_TREE to exec commands\n cache: update object ID functions for the_hash_algo\n tree-walk: replace hard-coded constants with the_hash_algo\n hex: switch to using the_hash_algo\n commit: express tree entry constants in terms of the_hash_algo\n strbuf: allocate space with GIT_MAX_HEXSZ\n sha1-name: use the_hash_algo when parsing object names\n refs/files-backend: use the_hash_algo for writing refs\n builtin/update-index: convert to using the_hash_algo\n builtin/update-index: simplify parsing of cacheinfo\n builtin/fmt-merge-msg: make hash independent\n builtin/merge: switch to use the_hash_algo\n builtin/merge-recursive: make hash independent\n diff: switch GIT_SHA1_HEXSZ to use the_hash_algo\n log-tree: switch GIT_SHA1_HEXSZ to the_hash_algo->hexsz\n sha1-file: convert constants to uses of the_hash_algo\n pretty: switch hard-coded constants to the_hash_algo\n\nÆvar Arnfjörð Bjarmason (36):\n checkout tests: index should be clean after dwim checkout\n checkout.h: wrap the arguments to unique_tracking_name()\n checkout.c: introduce an *_INIT macro\n checkout.c: change \"unique\" member to \"num_matches\"\n checkout: pass the \"num_matches\" up to callers\n builtin/checkout.c: use \"ret\" variable for return\n checkout: add advice for ambiguous \"checkout \"\n checkout & worktree: introduce checkout.defaultRemote\n refspec: s/refspec_item_init/&_or_die/g\n refspec: add back a refspec_item_init() function\n doc hash-function-transition: note the lack of a changelog\n receive.fsck. tests: remove dead code\n config doc: don't describe *.fetchObjects twice\n config doc: unify the description of fsck.* and receive.fsck.*\n config doc: elaborate on what transfer.fsckObjects does\n config doc: elaborate on fetch.fsckObjects security\n transfer.fsckObjects tests: untangle confusing setup\n fetch: implement fetch.fsck.*\n fsck: test & document {fetch,receive}.fsck.* config fallback\n fsck: add stress tests for fsck.skipList\n fsck: test and document unknown fsck. values\n tests: make use of the test_must_be_empty function\n tests: make use of the test_must_be_empty function\n fetch tests: change \"Tag\" test tag to \"testTag\"\n push tests: remove redundant 'git push' invocation\n push tests: fix logic error in \"push\" test assertion\n push tests: add more testing for forced tag pushing\n push tests: assert re-pushing annotated tags\n negotiator: unknown fetch.negotiationAlgorithm should error out\n fetch doc: cross-link two new negotiation options\n sha1dc: update from upstream\n push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets\n fetch tests: correct a comment \"remove it\" -> \"remove them\"\n pull doc: fix a long-standing grammar error\n submodule: add more exhaustive up-path testing\n t2024: mark test using \"checkout -p\" with PERL prerequisite\n\nŁukasz Stelmach (1):\n completion: complete remote names too\n\n"},{"id":"356147","messageId":"CAGZ79kbrv62ttBYt7nwO7E4S7wTVWZGceqE6hPjAbhPPWfRkEQ@mail.gmail.com","threadId":"49173","inReplyTo":"xmqqwoskadpe.fsf@gitster-ct.c.googlers.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T22:41:21Z","receivedAt":"2018-08-20T22:41:35Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> * The conversion to pass \"the_repository\" and then \"a_repository\"\n> throughout the object access API continues.\n>\n[...]\n>\n> * The conversion to pass \"the_repository\" and then \"a_repository\"\n> throughout the object access API continues.\n\nI guess it continues twice as two large series were merged? ;-)\nsb/object-store-grafts\nsb/object-store-lookup\n\nThe latter one is not the correct one, as later we'll have\n\n * lookup_commit_reference() and friends have been updated to find\n in-core object for a specific in-core repository instance.\n\n> * \"git submodule\" did not correctly adjust core.worktree setting that\n> indicates whether/where a submodule repository has its associated\n> working tree across various state transitions, which has been\n> corrected.\n> (merge 984cd77ddb sb/submodule-core-worktree later to maint).\n\nPersonally I do not view this as a bug fix but a feature\n(but then again my thinking might be tainted of too much\nsubmodule work) hence I would not merge it down.\n\nStefan\n"},{"id":"356150","messageId":"20180820233901.GC31020@aiede.svl.corp.google.com","threadId":"49173","inReplyTo":"CAGZ79kbrv62ttBYt7nwO7E4S7wTVWZGceqE6hPjAbhPPWfRkEQ@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-20T23:39:01Z","receivedAt":"2018-08-20T23:39:06Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(-cc: other lists)\nStefan Beller wrote:\n> Junio C Hamano wrote:\n\n>> * \"git submodule\" did not correctly adjust core.worktree setting that\n>> indicates whether/where a submodule repository has its associated\n>> working tree across various state transitions, which has been\n>> corrected.\n>> (merge 984cd77ddb sb/submodule-core-worktree later to maint).\n>\n> Personally I do not view this as a bug fix but a feature\n> (but then again my thinking might be tainted of too much\n> submodule work) hence I would not merge it down.\n\nCan you elaborate?\n\nThe symptom that this series fixes was pretty bad, so I'm pretty glad\nyou wrote it.\n\nThanks,\nJonathan\n"},{"id":"356151","messageId":"20180821002722.GA174626@aiede.svl.corp.google.com","threadId":"49173","inReplyTo":"20180820233901.GC31020@aiede.svl.corp.google.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-21T00:27:22Z","receivedAt":"2018-08-21T00:27:26Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>> Junio C Hamano wrote:\n\n>>> * \"git submodule\" did not correctly adjust core.worktree setting that\n>>> indicates whether/where a submodule repository has its associated\n>>> working tree across various state transitions, which has been\n>>> corrected.\n>>> (merge 984cd77ddb sb/submodule-core-worktree later to maint).\n>>\n>> Personally I do not view this as a bug fix but a feature\n>> (but then again my thinking might be tainted of too much\n>> submodule work) hence I would not merge it down.\n>\n> Can you elaborate?\n\n... ah, I figured it out. You are saying \"would not merge it down to\nmaint\". In that case, I agree, since this this is not a recent bug\n(it's existed since before v1.7.10-rc1~14^2~2, 2012-03-02).\n\nThanks,\nJonathan\n"},{"id":"356154","messageId":"CAGZ79kbAmt9qxcoo+hhcZzcGxQ3AxhMLdzmX3ZfL1GzvEBZb2w@mail.gmail.com","threadId":"49173","inReplyTo":"20180821002722.GA174626@aiede.svl.corp.google.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-21T00:46:13Z","receivedAt":"2018-08-21T00:46:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 5:27 PM Jonathan Nieder wrote:\n>\n> Jonathan Nieder wrote:\n> > Stefan Beller wrote:\n> >> Junio C Hamano wrote:\n>\n> >>> * \"git submodule\" did not correctly adjust core.worktree setting that\n> >>> indicates whether/where a submodule repository has its associated\n> >>> working tree across various state transitions, which has been\n> >>> corrected.\n> >>> (merge 984cd77ddb sb/submodule-core-worktree later to maint).\n> >>\n> >> Personally I do not view this as a bug fix but a feature\n> >> (but then again my thinking might be tainted of too much\n> >> submodule work) hence I would not merge it down.\n> >\n> > Can you elaborate?\n>\n> ... ah, I figured it out. You are saying \"would not merge it down to\n> maint\". In that case, I agree, since this this is not a recent bug\n> (it's existed since before v1.7.10-rc1~14^2~2, 2012-03-02).\n\nYeah; the behavior was the gold standard for submodules ever since,\nso I am wary of changing it under the guise of fixing a bug.\nThe core.worktree setting doesn't harm the user by default; you\nneed to craft a very specific situation to benefit from this feature.\n\nStefan\n"},{"id":"356212","messageId":"1b20b754-987c-a712-2594-235b845bc5d0@gmail.com","threadId":"49173","inReplyTo":"xmqqwoskadpe.fsf@gitster-ct.c.googlers.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T20:41:02Z","receivedAt":"2018-08-21T20:41:06Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2018 6:13 PM, Junio C Hamano wrote:\n> An early preview release Git v2.19.0-rc0 is now available for\n> testing at the usual places.\n\nAs part of testing the release candidate, I ran the performance suite \nagainst a fresh clone of the Linux repository using v2.18.0 and \nv2.19.0-rc0 (also: GIT_PERF_REPEAT_COUNT=10). I found a few nice \nimprovements, but I also found a possible regression in tree walking. I \nsay \"tree walking\" because it was revealed using p0001-rev-list.sh, but \nonly with the \"--objects\" flag. I also saw some similar numbers on 'git \nlog --raw'.\n\nTest v2.18.0             v2.19.0-rc0\n--------------------------------------------------------------------------------------------\n0001.1: rev-list --all 6.69(6.33+0.35)     6.52(6.20+0.31) -2.5%\n0001.2: rev-list --all --objects 52.14(47.43+1.02)   57.15(51.09+1.18) +9.6%\n\nTo me, 9.6% seems out of the range of just noise for this length of a \ncommand, but I could be wrong. Could anyone else try to repro these results?\n\n(This may also not just be tree-walking, but general pack-file loading \nand decompression, since I computed and stored a commit-graph file. \nHence, commits are not being parsed from the pack-file by either command.)\n\nAside: the perf results were not all bad. Here was an interesting \nimprovement:\n\nTest v2.18.0             v2.19.0-rc0\n--------------------------------------------------------------------------------------------\n0002.1: read_cache/discard_cache 1000 times 5.63(5.30+0.32)       \n3.34(3.03+0.30) -40.7%\n\nThanks,\n\n-Stolee\n\n"},{"id":"356218","messageId":"20180821212923.GB24431@sigill.intra.peff.net","threadId":"49173","inReplyTo":"1b20b754-987c-a712-2594-235b845bc5d0@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T21:29:24Z","receivedAt":"2018-08-21T21:29:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 04:41:02PM -0400, Derrick Stolee wrote:\n\n> On 8/20/2018 6:13 PM, Junio C Hamano wrote:\n> > An early preview release Git v2.19.0-rc0 is now available for\n> > testing at the usual places.\n> \n> As part of testing the release candidate, I ran the performance suite\n> against a fresh clone of the Linux repository using v2.18.0 and v2.19.0-rc0\n> (also: GIT_PERF_REPEAT_COUNT=10).\n\nWow, you're a glutton for punishment. :)\n\n> I found a few nice improvements, but I\n> also found a possible regression in tree walking. I say \"tree walking\"\n> because it was revealed using p0001-rev-list.sh, but only with the\n> \"--objects\" flag. I also saw some similar numbers on 'git log --raw'.\n> \n> Test v2.18.0             v2.19.0-rc0\n> --------------------------------------------------------------------------------------------\n> 0001.1: rev-list --all 6.69(6.33+0.35)     6.52(6.20+0.31) -2.5%\n> 0001.2: rev-list --all --objects 52.14(47.43+1.02)   57.15(51.09+1.18) +9.6%\n> \n> To me, 9.6% seems out of the range of just noise for this length of a\n> command, but I could be wrong. Could anyone else try to repro these results?\n\nI got:\n\n0001.2: rev-list --all --objects 37.07(36.62+0.45) 39.11(38.58+0.51) +5.5%\n\nLess change, but my overall times were smaller, too, so clearly our\nhardware or exact repos are a little bit different. Those numbers seem\npretty consistent in further runs.\n\nIt bisects to 509f6f62a4 (cache: update object ID functions for\nthe_hash_algo, 2018-07-16). Which make sense. An \"--objects\" traversal\nspends a huge amount of time checking each tree entry to see if we've\nprocessed that object yet, which ends up as hashcmp() in the hash table.\nI expect that a fixed 20-byte memcmp() can be optimized a lot more than\none with an arbitrary value.\n\nEven if _we_ know the value can only take on one of a few values, I\ndon't know that we have an easy way to tell the compiler that. Possibly\nwe could improve things by jumping directly to an optimized code path.\nSort of a poor-man's JIT. ;)\n\nDoing this:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..9c004a26c9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,7 +1023,10 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tif (the_hash_algo->rawsz == 20)\n+\t\treturn memcmp(sha1, sha2, 20);\n+\telse\n+\t\treturn memcmp(sha1, sha1, the_hash_algo->rawsz);\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\non top of v2.19-rc0 seems to give me about a 3% speedup (though I might\nbe imaging it, as there's a bit of noise). A function pointer in\nthe_hash_algo might make even more sense.\n\n-Peff\n"},{"id":"356227","messageId":"20180822004815.GA535143@genre.crustytoothpaste.net","threadId":"49173","inReplyTo":"20180821212923.GB24431@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-22T00:48:16Z","receivedAt":"2018-08-22T00:48:26Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Aug 21, 2018 at 05:29:24PM -0400, Jeff King wrote:\n> 0001.2: rev-list --all --objects 37.07(36.62+0.45) 39.11(38.58+0.51) +5.5%\n> \n> Less change, but my overall times were smaller, too, so clearly our\n> hardware or exact repos are a little bit different. Those numbers seem\n> pretty consistent in further runs.\n> \n> It bisects to 509f6f62a4 (cache: update object ID functions for\n> the_hash_algo, 2018-07-16). Which make sense. An \"--objects\" traversal\n> spends a huge amount of time checking each tree entry to see if we've\n> processed that object yet, which ends up as hashcmp() in the hash table.\n> I expect that a fixed 20-byte memcmp() can be optimized a lot more than\n> one with an arbitrary value.\n> \n> Even if _we_ know the value can only take on one of a few values, I\n> don't know that we have an easy way to tell the compiler that. Possibly\n> we could improve things by jumping directly to an optimized code path.\n> Sort of a poor-man's JIT. ;)\n> \n> Doing this:\n> \n> diff --git a/cache.h b/cache.h\n> index b1fd3d58ab..9c004a26c9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1023,7 +1023,10 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\tif (the_hash_algo->rawsz == 20)\n> +\t\treturn memcmp(sha1, sha2, 20);\n> +\telse\n> +\t\treturn memcmp(sha1, sha1, the_hash_algo->rawsz);\n> }\n> \n> static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n> on top of v2.19-rc0 seems to give me about a 3% speedup (though I might\n> be imaging it, as there's a bit of noise). A function pointer in\n> the_hash_algo might make even more sense.\n\nIt's possible that might be a better solution. I looked into a GCC\nassertion that the value was either 20 or 32, and that in itself didn't\nseem to help, at least in the generated code. Your solution is likely\nbetter in that regard.\n\nWe could wire it up to be either 20 or 32 and let people experimenting\nwith other sizes of algorithms just add another branch. I haven't\ntested how that performs, though.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"356229","messageId":"20180822030344.GA14684@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180822004815.GA535143@genre.crustytoothpaste.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T03:03:44Z","receivedAt":"2018-08-22T03:03:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 12:48:16AM +0000, brian m. carlson wrote:\n\n> > diff --git a/cache.h b/cache.h\n> > index b1fd3d58ab..9c004a26c9 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1023,7 +1023,10 @@ extern const struct object_id null_oid;\n> > \n> > static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> > {\n> > -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> > +\tif (the_hash_algo->rawsz == 20)\n> > +\t\treturn memcmp(sha1, sha2, 20);\n> > +\telse\n> > +\t\treturn memcmp(sha1, sha1, the_hash_algo->rawsz);\n> > }\n> > \n> > static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n> > on top of v2.19-rc0 seems to give me about a 3% speedup (though I might\n> > be imaging it, as there's a bit of noise). A function pointer in\n> > the_hash_algo might make even more sense.\n> \n> It's possible that might be a better solution. I looked into a GCC\n> assertion that the value was either 20 or 32, and that in itself didn't\n> seem to help, at least in the generated code. Your solution is likely\n> better in that regard.\n> \n> We could wire it up to be either 20 or 32 and let people experimenting\n> with other sizes of algorithms just add another branch. I haven't\n> tested how that performs, though.\n\nHere's a _really_ dirty one:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..a6750524ea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,6 +1023,7 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n+\tassert(the_hash_algo->rawsz == 20);\n \treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n }\n \n\nWe probably don't want to do that, because it makes experimenting with\nnew hash algos a bit painful, but it gives the same 3-4% speedup pretty\nconsistently. But I think it demonstrates pretty clearly that giving the\ncompiler the extra limit information is sufficient. Presumably the\nfixed-size memcmp turns into a few multi-word compares.\n\nAnd indeed, if I look at the generated asm for the call in lookup_object\n(which is likely the one we're hitting a lot in this case), I see:\n\n # cache.h:1027: return memcmp(sha1, sha2, the_hash_algo->rawsz);\n .loc 4 1027 9 is_stmt 0 view .LVU86\n movq (%rsi), %rcx # MEM[(void *)sha1_25(D)], MEM[(void *)sha1_25(D)]\n movq 8(%rsi), %rdi # MEM[(void *)sha1_25(D)], tmp125\n xorq 4(%rax), %rcx # MEM[(void *)_6], tmp116\n xorq 8(%r8), %rdi # MEM[(void *)_6], tmp115\n orq %rcx, %rdi # tmp116, tmp115\n jne .L27 #,\n movl 16(%r8), %ecx # MEM[(void *)_6], tmp129\n cmpl %ecx, 16(%rsi) # tmp129, MEM[(void *)sha1_25(D)]\n jne .L27 #,\n\nSo I wonder if there's some other way to tell the compiler that we'll\nonly have a few values. An enum comes to mind, though I don't think the\nenum rules are strict enough to make this guarantee (after all, it's OK\nto bitwise-OR enums, so they clearly don't specify all possible values).\n\nHaving a dedicate hashcmp function for each hash_algo seems like the\nsanest approach. We pay for one indirect function call, but the function\nitself will have the constants available. But it does introduce one\nextra complication. We're benefiting here from knowing that the size is\nalways 20, but also the inline hashcmp knows that we only care about\nequality, not comparison.\n\nSo if I do this:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..da56da7be2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,7 +1023,7 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\treturn the_hash_algo->cmp_fn(sha1, sha2);\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\ndiff --git a/hash.h b/hash.h\nindex 7c8238bc2e..ac22ba63b6 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -64,6 +64,7 @@ typedef union git_hash_ctx git_hash_ctx;\n typedef void (*git_hash_init_fn)(git_hash_ctx *ctx);\n typedef void (*git_hash_update_fn)(git_hash_ctx *ctx, const void *in, size_t len);\n typedef void (*git_hash_final_fn)(unsigned char *hash, git_hash_ctx *ctx);\n+typedef int (*git_hash_cmp_fn)(const void *a, const void *b);\n \n struct git_hash_algo {\n \t/*\n@@ -90,6 +91,8 @@ struct git_hash_algo {\n \t/* The hash finalization function. */\n \tgit_hash_final_fn final_fn;\n \n+\tgit_hash_cmp_fn cmp_fn;\n+\n \t/* The OID of the empty tree. */\n \tconst struct object_id *empty_tree;\n \ndiff --git a/sha1-file.c b/sha1-file.c\nindex 97b7423848..7072e360d7 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -69,6 +69,11 @@ static void git_hash_sha1_final(unsigned char *hash, git_hash_ctx *ctx)\n \tgit_SHA1_Final(hash, &ctx->sha1);\n }\n \n+static int git_hash_sha1_cmp(const void *a, const void *b)\n+{\n+\treturn memcmp(a, b, 20);\n+}\n+\n static void git_hash_unknown_init(git_hash_ctx *ctx)\n {\n \tBUG(\"trying to init unknown hash\");\n@@ -84,6 +89,11 @@ static void git_hash_unknown_final(unsigned char *hash, git_hash_ctx *ctx)\n \tBUG(\"trying to finalize unknown hash\");\n }\n \n+static int git_hash_unknown_cmp(const void *a, const void *b)\n+{\n+\tBUG(\"trying to compare unknown hash\");\n+}\n+\n const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t{\n \t\tNULL,\n@@ -93,6 +103,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t\tgit_hash_unknown_init,\n \t\tgit_hash_unknown_update,\n \t\tgit_hash_unknown_final,\n+\t\tgit_hash_unknown_cmp,\n \t\tNULL,\n \t\tNULL,\n \t},\n@@ -105,6 +116,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t\tgit_hash_sha1_init,\n \t\tgit_hash_sha1_update,\n \t\tgit_hash_sha1_final,\n+\t\tgit_hash_sha1_cmp,\n \t\t&empty_tree_oid,\n \t\t&empty_blob_oid,\n \t},\n\nthe result is actually _slower_ than the current code.\n\nIf I instead introduce an \"eq\" function, then that function can do the\noptimized thing. So if I do something more like this:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..52533a9710 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1026,6 +1026,11 @@ static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n \treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n }\n \n+static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)\n+{\n+\treturn the_hash_algo->eq_fn(sha1, sha2);\n+}\n+\n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n {\n \treturn hashcmp(oid1->hash, oid2->hash);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 97b7423848..a491ff5bef 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -69,6 +69,11 @@ static void git_hash_sha1_final(unsigned char *hash, git_hash_ctx *ctx)\n \tgit_SHA1_Final(hash, &ctx->sha1);\n }\n \n+static int git_hash_sha1_eq(const void *a, const void *b)\n+{\n+\treturn !memcmp(a, b, 20);\n+}\n+\n static void git_hash_unknown_init(git_hash_ctx *ctx)\n {\n \tBUG(\"trying to init unknown hash\");\n> [omitting similar hunks from the last one]\n\nplus converting this one callsite:\n\ndiff --git a/object.c b/object.c\nindex 51c4594515..e54160550c 100644\n--- a/object.c\n+++ b/object.c\n@@ -95,7 +95,7 @@ struct object *lookup_object(struct repository *r, const unsigned char *sha1)\n \n \tfirst = i = hash_obj(sha1, r->parsed_objects->obj_hash_size);\n \twhile ((obj = r->parsed_objects->obj_hash[i]) != NULL) {\n-\t\tif (!hashcmp(sha1, obj->oid.hash))\n+\t\tif (hasheq(sha1, obj->oid.hash))\n \t\t\tbreak;\n \t\ti++;\n \t\tif (i == r->parsed_objects->obj_hash_size)\n\nI get about a 1.5% speedup. If I apply this coccinelle patch:\n\n@@\nexpression a, b;\n@@\n- !hashcmp(a, b)\n+ hasheq(a, b)\n\n@@\nexpression a, b;\n@@\n- !oidcmp(a, b)\n+ oideq(a, b)\n\nwith the obvious \"oideq()\" implementation added, that seems to get me to\n2-3%. Not _quite_ as good as the original branching version I showed.\nAnd we had to touch all the callsites (although arguably that kind of\n\"eq\" function is a better interface anyway, since it obviously allows\nfor more optimization.\n\nSo maybe the branching thing is actually not so insane. It makes new\nhash_algo's Just Work; they just won't be optimized. And the change is\nvery localized.\n\nOr maybe it's crazy to spend any effort at all chasing a few percent.\nIt's not like people's large repositories aren't just going to grow by\nthat much after a few months anyway. ;) It just seems like if we can do\nit for little cost, it's worth it.\n\n-Peff\n"},{"id":"356230","messageId":"20180822033654.GA30196@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180822030344.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T03:36:54Z","receivedAt":"2018-08-22T03:36:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n\n> with the obvious \"oideq()\" implementation added, that seems to get me to\n> 2-3%. Not _quite_ as good as the original branching version I showed.\n> And we had to touch all the callsites (although arguably that kind of\n> \"eq\" function is a better interface anyway, since it obviously allows\n> for more optimization.\n> \n> So maybe the branching thing is actually not so insane. It makes new\n> hash_algo's Just Work; they just won't be optimized. And the change is\n> very localized.\n\nHmph. So I went back to double-check my measurements on that branching\nversion, and I couldn't replicate it!\n\nIt turns out what I showed (and measured) before has a bug. Can you see\nit?\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..9c004a26c9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,7 +1023,10 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tif (the_hash_algo->rawsz == 20)\n+\t\treturn memcmp(sha1, sha2, 20);\n+\telse\n+\t\treturn memcmp(sha1, sha1, the_hash_algo->rawsz);\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n\n\nThe problem is the fallback code compares \"sha1\" to \"sha1\". The compiler\nrealizes that's a noop and is able to treat it like a constant. Thus\nessentially leaving only the first branch, which it then expands into a\nfew instructions.\n\nIf we fix that bug, then we really do memcmp on either side of the\nconditional. And the compiler is smart enough to realize that hey,\nthat's the same as just calling memcmp with the_hash_algo->rawsz on\neither side. And we end up with roughly the same code that we started\nwith.\n\nSo the assert() version really is the fastest. I didn't test, but I\nsuspect we could \"trick\" the compiler by having the fallback call an\nopaque wrapper around memcmp(). That would prevent it from combining the\ntwo paths, and presumably it would still optimize the constant-20 side.\nOr maybe it would eventually decide our inline function is getting too\nbig and scrap it. Which probably crosses a line of craziness (if I\ndidn't already cross it two emails ago).\n\n-Peff\n"},{"id":"356231","messageId":"20180822053626.GB535143@genre.crustytoothpaste.net","threadId":"49173","inReplyTo":"20180822030344.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-22T05:36:26Z","receivedAt":"2018-08-22T05:36:36Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n> So I wonder if there's some other way to tell the compiler that we'll\n> only have a few values. An enum comes to mind, though I don't think the\n> enum rules are strict enough to make this guarantee (after all, it's OK\n> to bitwise-OR enums, so they clearly don't specify all possible values).\n\nI was thinking about this:\n\ndiff --git a/cache.h b/cache.h\nindex 1398b2a4e4..1f5c6e9319 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1033,7 +1033,14 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tswitch (the_hash_algo->rawsz) {\n+\t\tcase 20:\n+\t\t\treturn memcmp(sha1, sha2, 20);\n+\t\tcase 32:\n+\t\t\treturn memcmp(sha1, sha2, 32);\n+\t\tdefault:\n+\t\t\tassert(0);\n+\t}\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n\nThat would make it obvious that there are at most two options.\nUnfortunately, gcc for me determines that the buffer in walker.c is 20\nbytes in size and steadfastly refuses to compile because it doesn't know\nthat the value will never be 32 in our codebase currently. I'd need to\nsend in more patches before it would compile.\n\nI don't know if something like this is an improvement or now, but this\nseems to at least compile:\n\ndiff --git a/cache.h b/cache.h\nindex 1398b2a4e4..3207f74771 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tswitch (the_hash_algo->rawsz) {\n+\t\tcase 20:\n+\t\tcase 32:\n+\t\t\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\t\tdefault:\n+\t\t\tassert(0);\n+\t}\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n\nI won't have time to sit down and test this out until tomorrow afternoon\nat the earliest. If you want to send in something in the mean time,\neven if that limits things to just 20 for now, that's fine.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"356234","messageId":"20180822060735.GA13195@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180822053626.GB535143@genre.crustytoothpaste.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T06:07:35Z","receivedAt":"2018-08-22T06:07:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 05:36:26AM +0000, brian m. carlson wrote:\n\n> On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n> > So I wonder if there's some other way to tell the compiler that we'll\n> > only have a few values. An enum comes to mind, though I don't think the\n> > enum rules are strict enough to make this guarantee (after all, it's OK\n> > to bitwise-OR enums, so they clearly don't specify all possible values).\n> \n> I was thinking about this:\n> \n> diff --git a/cache.h b/cache.h\n> index 1398b2a4e4..1f5c6e9319 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1033,7 +1033,14 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\tswitch (the_hash_algo->rawsz) {\n> +\t\tcase 20:\n> +\t\t\treturn memcmp(sha1, sha2, 20);\n> +\t\tcase 32:\n> +\t\t\treturn memcmp(sha1, sha2, 32);\n> +\t\tdefault:\n> +\t\t\tassert(0);\n> +\t}\n> }\n\nUnfortunately this version doesn't seem to be any faster than the status\nquo. And looking at the generated asm, it still looks to be calling\nmemcpy(). Removing the \"case 32\" branch switches it back to fast\nassembly (this is all using gcc 8.2.0, btw). So I think we're deep into\nguessing what the optimizer is going to do, and there's a good chance\nthat other versions are going to optimize it differently.\n\nWe might be better off just writing it out manually. Unfortunately, it's\na bit hard because the neg/0/pos return is more expensive to compute\nthan pure equality. And only the compiler knows at each inlined site\nwhether we actually want equality. So now we're back to switching every\ncaller to use hasheq() if that's what they want.\n\nBut _if_ we're OK with that, and _if_ we don't mind some ifdefs for\nportability, then this seems as fast as the original (memcmp+constant)\ncode on my machine:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..c406105f3c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,7 +1023,16 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tswitch (the_hash_algo->rawsz) {\n+\tcase 20:\n+\t\tif (*(uint32_t *)sha1 == *(uint32_t *)sha2 &&\n+\t\t *(unsigned __int128 *)(sha1+4) == *(unsigned __int128 *)(sha2+4))\n+\t\t\treturn 0;\n+\tcase 32:\n+\t\treturn memcmp(sha1, sha2, 32);\n+\tdefault:\n+\t\tassert(0);\n+\t}\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n\nWhich is really no surprise, because the generated asm looks about the\nsame. There are obviously alignment questions there. It's possible it\ncould even be written portably as a simple loop. Or maybe not. We used\nto do that, but modern compilers were able to optimize the memcmp\nbetter. Maybe that's changed. Or maybe they were simply unwilling to\nunroll a 20-length loop to find out that it could be turned into a few\nquad-word compares.\n\n> That would make it obvious that there are at most two options.\n> Unfortunately, gcc for me determines that the buffer in walker.c is 20\n> bytes in size and steadfastly refuses to compile because it doesn't know\n> that the value will never be 32 in our codebase currently. I'd need to\n> send in more patches before it would compile.\n\nYeah, I see that warning all over the place (everywhere that calls\nis_null_oid(), which is passing in a 20-byte buffer).\n\n> I don't know if something like this is an improvement or now, but this\n> seems to at least compile:\n> \n> diff --git a/cache.h b/cache.h\n> index 1398b2a4e4..3207f74771 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\tswitch (the_hash_algo->rawsz) {\n> +\t\tcase 20:\n> +\t\tcase 32:\n> +\t\t\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\t\tdefault:\n> +\t\t\tassert(0);\n> +\t}\n\nI think that would end up with the same slow code, as gcc would rather\ncall memcmp than expand out the two sets of asm.\n\n> I won't have time to sit down and test this out until tomorrow afternoon\n> at the earliest. If you want to send in something in the mean time,\n> even if that limits things to just 20 for now, that's fine.\n\nI don't have a good option. The assert() thing works until I add in the\n\"32\" branch, but that's just punting the issue off until you add support\nfor the new hash.\n\nHand-rolling our own asm or C is a portability headache, and we need to\nchange all of the callsites to use a new hasheq().\n\nHiding it behind a per-hash function is conceptually cleanest, but not\nquite as fast. And it also requires hasheq().\n\nSo all of the solutions seem non-trivial. Again, I'm starting to wonder\nif it's worth chasing this few percent.\n\n-Peff\n"},{"id":"356235","messageId":"CACBZZX7Cmp8d=UKF2nk36fL7mR+umdKwKZAKNZSkyP0NXvquhw@mail.gmail.com","threadId":"49173","inReplyTo":"20180822060735.GA13195@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-22T07:39:57Z","receivedAt":"2018-08-22T07:40:12Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Wed, Aug 22, 2018 at 8:20 AM Jeff King wrote:\n>\n> On Wed, Aug 22, 2018 at 05:36:26AM +0000, brian m. carlson wrote:\n>\n> > On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n> > > So I wonder if there's some other way to tell the compiler that we'll\n> > > only have a few values. An enum comes to mind, though I don't think the\n> > > enum rules are strict enough to make this guarantee (after all, it's OK\n> > > to bitwise-OR enums, so they clearly don't specify all possible values).\n> >\n> > I was thinking about this:\n> >\n> > diff --git a/cache.h b/cache.h\n> > index 1398b2a4e4..1f5c6e9319 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1033,7 +1033,14 @@ extern const struct object_id null_oid;\n> >\n> > static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> > {\n> > - return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> > + switch (the_hash_algo->rawsz) {\n> > + case 20:\n> > + return memcmp(sha1, sha2, 20);\n> > + case 32:\n> > + return memcmp(sha1, sha2, 32);\n> > + default:\n> > + assert(0);\n> > + }\n> > }\n>\n> Unfortunately this version doesn't seem to be any faster than the status\n> quo. And looking at the generated asm, it still looks to be calling\n> memcpy(). Removing the \"case 32\" branch switches it back to fast\n> assembly (this is all using gcc 8.2.0, btw). So I think we're deep into\n> guessing what the optimizer is going to do, and there's a good chance\n> that other versions are going to optimize it differently.\n>\n> We might be better off just writing it out manually. Unfortunately, it's\n> a bit hard because the neg/0/pos return is more expensive to compute\n> than pure equality. And only the compiler knows at each inlined site\n> whether we actually want equality. So now we're back to switching every\n> caller to use hasheq() if that's what they want.\n>\n> But _if_ we're OK with that, and _if_ we don't mind some ifdefs for\n> portability, then this seems as fast as the original (memcmp+constant)\n> code on my machine:\n>\n> diff --git a/cache.h b/cache.h\n> index b1fd3d58ab..c406105f3c 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1023,7 +1023,16 @@ extern const struct object_id null_oid;\n>\n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> - return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> + switch (the_hash_algo->rawsz) {\n> + case 20:\n> + if (*(uint32_t *)sha1 == *(uint32_t *)sha2 &&\n> + *(unsigned __int128 *)(sha1+4) == *(unsigned __int128 *)(sha2+4))\n> + return 0;\n> + case 32:\n> + return memcmp(sha1, sha2, 32);\n> + default:\n> + assert(0);\n> + }\n> }\n>\n> static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n>\n> Which is really no surprise, because the generated asm looks about the\n> same. There are obviously alignment questions there. It's possible it\n> could even be written portably as a simple loop. Or maybe not. We used\n> to do that, but modern compilers were able to optimize the memcmp\n> better. Maybe that's changed. Or maybe they were simply unwilling to\n> unroll a 20-length loop to find out that it could be turned into a few\n> quad-word compares.\n>\n> > That would make it obvious that there are at most two options.\n> > Unfortunately, gcc for me determines that the buffer in walker.c is 20\n> > bytes in size and steadfastly refuses to compile because it doesn't know\n> > that the value will never be 32 in our codebase currently. I'd need to\n> > send in more patches before it would compile.\n>\n> Yeah, I see that warning all over the place (everywhere that calls\n> is_null_oid(), which is passing in a 20-byte buffer).\n>\n> > I don't know if something like this is an improvement or now, but this\n> > seems to at least compile:\n> >\n> > diff --git a/cache.h b/cache.h\n> > index 1398b2a4e4..3207f74771 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;\n> >\n> > static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> > {\n> > - return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> > + switch (the_hash_algo->rawsz) {\n> > + case 20:\n> > + case 32:\n> > + return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> > + default:\n> > + assert(0);\n> > + }\n>\n> I think that would end up with the same slow code, as gcc would rather\n> call memcmp than expand out the two sets of asm.\n>\n> > I won't have time to sit down and test this out until tomorrow afternoon\n> > at the earliest. If you want to send in something in the mean time,\n> > even if that limits things to just 20 for now, that's fine.\n>\n> I don't have a good option. The assert() thing works until I add in the\n> \"32\" branch, but that's just punting the issue off until you add support\n> for the new hash.\n>\n> Hand-rolling our own asm or C is a portability headache, and we need to\n> change all of the callsites to use a new hasheq().\n>\n> Hiding it behind a per-hash function is conceptually cleanest, but not\n> quite as fast. And it also requires hasheq().\n>\n> So all of the solutions seem non-trivial. Again, I'm starting to wonder\n> if it's worth chasing this few percent.\n\nDid you try __builtin_expect? It's a GCC builtin for these sorts of\nsituations, and sometimes helps:\nhttps://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html\n\nI.e. you'd tell GCC we expect to have the 20 there with:\n\n if (__builtin_expect(the_hash_algo->rawsz == 20, 1)) { ... }\n\nThe perl codebase has LIKELY() and UNLIKELY() macros for this which if\nthe feature isn't available fall back on just plain C code:\nhttps://github.com/Perl/perl5/blob/v5.27.7/perl.h#L3335-L3344\n"},{"id":"356240","messageId":"b60e8d18-261c-c86d-b5e0-b06a5d46d9ea@gmail.com","threadId":"49173","inReplyTo":"20180822033654.GA30196@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T11:11:54Z","receivedAt":"2018-08-22T11:12:00Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 11:36 PM, Jeff King wrote:\n> On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n>\n>> with the obvious \"oideq()\" implementation added, that seems to get me to\n>> 2-3%. Not _quite_ as good as the original branching version I showed.\n>> And we had to touch all the callsites (although arguably that kind of\n>> \"eq\" function is a better interface anyway, since it obviously allows\n>> for more optimization.\n>>\n>> So maybe the branching thing is actually not so insane. It makes new\n>> hash_algo's Just Work; they just won't be optimized. And the change is\n>> very localized.\n> Hmph. So I went back to double-check my measurements on that branching\n> version, and I couldn't replicate it!\nI'm actually relieved to see this, as I couldn't either.\n>\n> It turns out what I showed (and measured) before has a bug. Can you see\n> it?\nI had rewritten the section from scratch instead of applying your diff, \nso I didn't get the sha1-sha1 error. I decided to sleep on it instead of \nsending my email.\n\n> So the assert() version really is the fastest. I didn't test, but I\n> suspect we could \"trick\" the compiler by having the fallback call an\n> opaque wrapper around memcmp(). That would prevent it from combining the\n> two paths, and presumably it would still optimize the constant-20 side.\n> Or maybe it would eventually decide our inline function is getting too\n> big and scrap it. Which probably crosses a line of craziness (if I\n> didn't already cross it two emails ago).\nI appreciate your effort here.\n\nThanks\n-Stolee\n"},{"id":"356241","messageId":"d1fafc75-d6e4-d363-d600-579c200aca43@gmail.com","threadId":"49173","inReplyTo":"CACBZZX7Cmp8d=UKF2nk36fL7mR+umdKwKZAKNZSkyP0NXvquhw@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T11:14:42Z","receivedAt":"2018-08-22T11:14:47Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\nOn 8/22/2018 3:39 AM, Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Aug 22, 2018 at 8:20 AM Jeff King wrote:\n>> On Wed, Aug 22, 2018 at 05:36:26AM +0000, brian m. carlson wrote:\n>>\n>>> On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n>>> I don't know if something like this is an improvement or now, but this\n>>> seems to at least compile:\n>>>\n>>> diff --git a/cache.h b/cache.h\n>>> index 1398b2a4e4..3207f74771 100644\n>>> --- a/cache.h\n>>> +++ b/cache.h\n>>> @@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;\n>>>\n>>> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n>>> {\n>>> - return memcmp(sha1, sha2, the_hash_algo->rawsz);\n>>> + switch (the_hash_algo->rawsz) {\n>>> + case 20:\n>>> + case 32:\n>>> + return memcmp(sha1, sha2, the_hash_algo->rawsz);\n>>> + default:\n>>> + assert(0);\n>>> + }\n>> I think that would end up with the same slow code, as gcc would rather\n>> call memcmp than expand out the two sets of asm.\n>>\n>>> I won't have time to sit down and test this out until tomorrow afternoon\n>>> at the earliest. If you want to send in something in the mean time,\n>>> even if that limits things to just 20 for now, that's fine.\n>> I don't have a good option. The assert() thing works until I add in the\n>> \"32\" branch, but that's just punting the issue off until you add support\n>> for the new hash.\n>>\n>> Hand-rolling our own asm or C is a portability headache, and we need to\n>> change all of the callsites to use a new hasheq().\n>>\n>> Hiding it behind a per-hash function is conceptually cleanest, but not\n>> quite as fast. And it also requires hasheq().\n>>\n>> So all of the solutions seem non-trivial. Again, I'm starting to wonder\n>> if it's worth chasing this few percent.\n> Did you try __builtin_expect? It's a GCC builtin for these sorts of\n> situations, and sometimes helps:\n> https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html\n>\n> I.e. you'd tell GCC we expect to have the 20 there with:\n>\n> if (__builtin_expect(the_hash_algo->rawsz == 20, 1)) { ... }\n>\n> The perl codebase has LIKELY() and UNLIKELY() macros for this which if\n> the feature isn't available fall back on just plain C code:\n> https://github.com/Perl/perl5/blob/v5.27.7/perl.h#L3335-L3344\nThe other thing I was going to recommend (and I'll try to test this out \nmyself later) is to see if 'the_hash_algo->rawsz' is being treated as a \nvolatile variable, since it is being referenced through a pointer. \nPerhaps storing the value locally and then casing on it would help?\n"},{"id":"356248","messageId":"814549a01074e89a4b26cb0cf13e4dddeb3a040a.camel@mad-scientist.net","threadId":"49173","inReplyTo":"20180822030344.GA14684@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Paul Smith","fromEmail":"paul@mad-scientist.net","sentAt":"2018-08-22T12:42:20Z","receivedAt":"2018-08-22T13:41:13Z","isPatch":false,"sender":{"key":"paul@mad-scientist.net","avatar":"https://avatars.githubusercontent.com/u/109636?v=4"},"body":"On Tue, 2018-08-21 at 23:03 -0400, Jeff King wrote:\n> static inline int hashcmp(const unsigned char *sha1, const unsigned\n> char *sha2)\n> {\n> + assert(the_hash_algo->rawsz == 20);\n> return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> }\n\nI'm not familiar with Git code, but for most environments assert() is a\nmacro which is compiled out when built for \"release mode\" (whatever\nthat might mean). If that's the case for Git too, then relying on\nassert() to provide a side-effect (even an optimizer hint side-effect)\nwon't work and this will actually get slower when built for \"release\nmode\".\n\nJust a thought...\n"},{"id":"356249","messageId":"459f138f-78ba-6465-0f2b-b2419bbc36ff@gmail.com","threadId":"49173","inReplyTo":"20180822053626.GB535143@genre.crustytoothpaste.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T14:28:56Z","receivedAt":"2018-08-22T14:29:03Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2018 1:36 AM, brian m. carlson wrote:\n> On Tue, Aug 21, 2018 at 11:03:44PM -0400, Jeff King wrote:\n>> So I wonder if there's some other way to tell the compiler that we'll\n>> only have a few values. An enum comes to mind, though I don't think the\n>> enum rules are strict enough to make this guarantee (after all, it's OK\n>> to bitwise-OR enums, so they clearly don't specify all possible values).\n> I was thinking about this:\n>\n> diff --git a/cache.h b/cache.h\n> index 1398b2a4e4..1f5c6e9319 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1033,7 +1033,14 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\tswitch (the_hash_algo->rawsz) {\n> +\t\tcase 20:\n> +\t\t\treturn memcmp(sha1, sha2, 20);\n> +\t\tcase 32:\n> +\t\t\treturn memcmp(sha1, sha2, 32);\n> +\t\tdefault:\n> +\t\t\tassert(0);\n> +\t}\n> }\n> \n> static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\n>\n> That would make it obvious that there are at most two options.\n> Unfortunately, gcc for me determines that the buffer in walker.c is 20\n> bytes in size and steadfastly refuses to compile because it doesn't know\n> that the value will never be 32 in our codebase currently. I'd need to\n> send in more patches before it would compile.\n>\n> I don't know if something like this is an improvement or now, but this\n> seems to at least compile:\n>\n> diff --git a/cache.h b/cache.h\n> index 1398b2a4e4..3207f74771 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1033,7 +1033,13 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> -\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\tswitch (the_hash_algo->rawsz) {\n> +\t\tcase 20:\n> +\t\tcase 32:\n> +\t\t\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +\t\tdefault:\n> +\t\t\tassert(0);\n> +\t}\n> }\nIn my testing, I've had the best luck with this change:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..6c8b51c390 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,7 +1023,14 @@ extern const struct object_id null_oid;\n\n  static inline int hashcmp(const unsigned char *sha1, const unsigned \nchar *sha2)\n  {\n-       return memcmp(sha1, sha2, the_hash_algo->rawsz);\n+       switch (the_hash_algo->rawsz) {\n+               case 20:\n+                       return memcmp(sha1, sha2, 20);\n+               case 32:\n+                       return memcmp(sha1, sha2, 32);\n+               default:\n+                       assert(0);\n+       }\n  }\n\nThe fact that '20' and '32' are constants here may be helpful to the \ncompiler. Can someone else test the perf?\n\nThanks,\n-Stolee\n"},{"id":"356250","messageId":"20180822151407.GA32630@sigill.intra.peff.net","threadId":"49173","inReplyTo":"CACBZZX7Cmp8d=UKF2nk36fL7mR+umdKwKZAKNZSkyP0NXvquhw@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T15:14:07Z","receivedAt":"2018-08-22T15:14:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 09:39:57AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I don't have a good option. The assert() thing works until I add in the\n> > \"32\" branch, but that's just punting the issue off until you add support\n> > for the new hash.\n> >\n> > Hand-rolling our own asm or C is a portability headache, and we need to\n> > change all of the callsites to use a new hasheq().\n> >\n> > Hiding it behind a per-hash function is conceptually cleanest, but not\n> > quite as fast. And it also requires hasheq().\n> >\n> > So all of the solutions seem non-trivial. Again, I'm starting to wonder\n> > if it's worth chasing this few percent.\n> \n> Did you try __builtin_expect? It's a GCC builtin for these sorts of\n> situations, and sometimes helps:\n> https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html\n> \n> I.e. you'd tell GCC we expect to have the 20 there with:\n> \n> if (__builtin_expect(the_hash_algo->rawsz == 20, 1)) { ... }\n> \n> The perl codebase has LIKELY() and UNLIKELY() macros for this which if\n> the feature isn't available fall back on just plain C code:\n> https://github.com/Perl/perl5/blob/v5.27.7/perl.h#L3335-L3344\n\nSadly, no, this doesn't seem to change anything. We still end up with a\nsingle call to memcmp.\n\nI also tried \"hiding\" the fallback call like this:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..7808bf3d6b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1021,9 +1021,13 @@ extern int find_unique_abbrev_r(char *hex, const struct object_id *oid, int len)\n extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n extern const struct object_id null_oid;\n \n+int super_secret_memcmp(const void *a, const void *b, size_t len);\n+\n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n-\treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n+\tif (the_hash_algo->rawsz == 20)\n+\t\treturn memcmp(sha1, sha2, 20);\n+\treturn super_secret_memcmp(sha1, sha2, the_hash_algo->rawsz);\n }\n \n static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 97b7423848..5cd0a4b73f 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -2280,3 +2280,8 @@ int read_loose_object(const char *path,\n \t\tmunmap(map, mapsize);\n \treturn ret;\n }\n+\n+int super_secret_memcmp(const void *a, const void *b, size_t len)\n+{\n+\treturn memcmp(a, b, len);\n+}\n\nbut that just results in calling memcmp and super_secret_memcmp on the\ntwo codepaths (with or without the __builtin_expect).\n\n-Peff\n"},{"id":"356251","messageId":"20180822151703.GB32630@sigill.intra.peff.net","threadId":"49173","inReplyTo":"d1fafc75-d6e4-d363-d600-579c200aca43@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T15:17:04Z","receivedAt":"2018-08-22T15:17:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n\n> The other thing I was going to recommend (and I'll try to test this out\n> myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n> volatile variable, since it is being referenced through a pointer. Perhaps\n> storing the value locally and then casing on it would help?\n\nI tried various sprinkling of \"const\" around the declarations to make it\nclear that the values wouldn't change once we saw them. But I couldn't\ndetect any difference. At most I think that would let us hoist the \"if\"\nout of the loop, but gcc still seems unwilling to expand the memcmp when\nthere are other branches.\n\nI think if that's the thing we want to have happen, we really do need to\njust write it out on that branch rather than saying \"memcmp\".\n\n-Peff\n"},{"id":"356252","messageId":"20180822152306.GC32630@sigill.intra.peff.net","threadId":"49173","inReplyTo":"814549a01074e89a4b26cb0cf13e4dddeb3a040a.camel@mad-scientist.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T15:23:06Z","receivedAt":"2018-08-22T15:23:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 08:42:20AM -0400, Paul Smith wrote:\n\n> On Tue, 2018-08-21 at 23:03 -0400, Jeff King wrote:\n> > static inline int hashcmp(const unsigned char *sha1, const unsigned\n> > char *sha2)\n> > {\n> > + assert(the_hash_algo->rawsz == 20);\n> > return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> > }\n> \n> I'm not familiar with Git code, but for most environments assert() is a\n> macro which is compiled out when built for \"release mode\" (whatever\n> that might mean). If that's the case for Git too, then relying on\n> assert() to provide a side-effect (even an optimizer hint side-effect)\n> won't work and this will actually get slower when built for \"release\n> mode\".\n> \n> Just a thought...\n\nWe don't have such a \"release mode\" in Git, though of course people may\npass -DNDEBUG to the compiler if they want.\n\nHowever, to me how we spell the assert is mostly orthogonal to the\ndiscussion. We can do \"if (...) BUG(...)\" to get a guaranteed-present\nconditional. The bigger questions are:\n\n - are we OK with such an assertion; and\n\n - does the assertion still give us the desired behavior when we add in\n a branch for rawsz==32?\n\nAnd I think the answers for those are both \"probably not\".\n\n-Peff\n"},{"id":"356253","messageId":"20180822152453.GD32630@sigill.intra.peff.net","threadId":"49173","inReplyTo":"459f138f-78ba-6465-0f2b-b2419bbc36ff@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T15:24:54Z","receivedAt":"2018-08-22T15:24:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 10:28:56AM -0400, Derrick Stolee wrote:\n\n> In my testing, I've had the best luck with this change:\n> \n> diff --git a/cache.h b/cache.h\n> index b1fd3d58ab..6c8b51c390 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1023,7 +1023,14 @@ extern const struct object_id null_oid;\n> \n>  static inline int hashcmp(const unsigned char *sha1, const unsigned char\n> *sha2)\n>  {\n> -       return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +       switch (the_hash_algo->rawsz) {\n> +               case 20:\n> +                       return memcmp(sha1, sha2, 20);\n> +               case 32:\n> +                       return memcmp(sha1, sha2, 32);\n> +               default:\n> +                       assert(0);\n> +       }\n>  }\n> \n> The fact that '20' and '32' are constants here may be helpful to the\n> compiler. Can someone else test the perf?\n\nI tested that one last night (and just re-tested it now to be sure). It\nseems to just generate two separate calls to memcmp, with no speed\nimprovement.\n\n-Peff\n"},{"id":"356274","messageId":"CACsJy8B+FN6W32ZUgetXbEJ=Ld5ZDHHhNWVz2aiZLZG1_qA+FQ@mail.gmail.com","threadId":"49173","inReplyTo":"20180822151703.GB32630@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-22T16:08:38Z","receivedAt":"2018-08-22T16:09:07Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n>\n> On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n>\n> > The other thing I was going to recommend (and I'll try to test this out\n> > myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n> > volatile variable, since it is being referenced through a pointer. Perhaps\n> > storing the value locally and then casing on it would help?\n>\n> I tried various sprinkling of \"const\" around the declarations to make it\n> clear that the values wouldn't change once we saw them. But I couldn't\n> detect any difference. At most I think that would let us hoist the \"if\"\n> out of the loop, but gcc still seems unwilling to expand the memcmp when\n> there are other branches.\n>\n> I think if that's the thing we want to have happen, we really do need to\n> just write it out on that branch rather than saying \"memcmp\".\n\nThis reminds me of an old discussion about memcpy() vs doing explicit\ncompare loop with lots of performance measurements.. Is that what you\nmeant by \"write it out\"?\n-- \nDuy\n"},{"id":"356276","messageId":"CACsJy8DifmYtDE4D58yVM7rYhfJFe8p-t9bXgMmMsw=txxmzHA@mail.gmail.com","threadId":"49173","inReplyTo":"CACsJy8B+FN6W32ZUgetXbEJ=Ld5ZDHHhNWVz2aiZLZG1_qA+FQ@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-22T16:14:24Z","receivedAt":"2018-08-22T16:14:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen wrote:\n>\n> On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n> >\n> > On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n> >\n> > > The other thing I was going to recommend (and I'll try to test this out\n> > > myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n> > > volatile variable, since it is being referenced through a pointer. Perhaps\n> > > storing the value locally and then casing on it would help?\n> >\n> > I tried various sprinkling of \"const\" around the declarations to make it\n> > clear that the values wouldn't change once we saw them. But I couldn't\n> > detect any difference. At most I think that would let us hoist the \"if\"\n> > out of the loop, but gcc still seems unwilling to expand the memcmp when\n> > there are other branches.\n> >\n> > I think if that's the thing we want to have happen, we really do need to\n> > just write it out on that branch rather than saying \"memcmp\".\n>\n> This reminds me of an old discussion about memcpy() vs doing explicit\n> compare loop with lots of performance measurements..\n\nAh found it. Not sure if it is still relevant in light of multiple hash support\n\nhttps://public-inbox.org/git/20110427225114.GA16765@elte.hu/\n-- \nDuy\n"},{"id":"356277","messageId":"20180822162609.GA11904@sigill.intra.peff.net","threadId":"49173","inReplyTo":"CACsJy8DifmYtDE4D58yVM7rYhfJFe8p-t9bXgMmMsw=txxmzHA@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T16:26:09Z","receivedAt":"2018-08-22T16:26:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote:\n\n> On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen wrote:\n> >\n> > On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n> > >\n> > > On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n> > >\n> > > > The other thing I was going to recommend (and I'll try to test this out\n> > > > myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n> > > > volatile variable, since it is being referenced through a pointer. Perhaps\n> > > > storing the value locally and then casing on it would help?\n> > >\n> > > I tried various sprinkling of \"const\" around the declarations to make it\n> > > clear that the values wouldn't change once we saw them. But I couldn't\n> > > detect any difference. At most I think that would let us hoist the \"if\"\n> > > out of the loop, but gcc still seems unwilling to expand the memcmp when\n> > > there are other branches.\n> > >\n> > > I think if that's the thing we want to have happen, we really do need to\n> > > just write it out on that branch rather than saying \"memcmp\".\n> >\n> > This reminds me of an old discussion about memcpy() vs doing explicit\n> > compare loop with lots of performance measurements..\n> \n> Ah found it. Not sure if it is still relevant in light of multiple hash support\n> \n> https://public-inbox.org/git/20110427225114.GA16765@elte.hu/\n\nYes, that was what I meant. We actually did switch to that hand-rolled\nloop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\nmemcmp instead of open-coded loop, 2017-08-09).\n\n-Peff\n"},{"id":"356282","messageId":"7ea416cf-b043-1274-e161-85a8780b8e1c@gmail.com","threadId":"49173","inReplyTo":"20180822162609.GA11904@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T16:49:34Z","receivedAt":"2018-08-22T16:49:39Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2018 12:26 PM, Jeff King wrote:\n> On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote:\n>\n>> On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen wrote:\n>>> On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n>>>> On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n>>>>\n>>>>> The other thing I was going to recommend (and I'll try to test this out\n>>>>> myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n>>>>> volatile variable, since it is being referenced through a pointer. Perhaps\n>>>>> storing the value locally and then casing on it would help?\n>>>> I tried various sprinkling of \"const\" around the declarations to make it\n>>>> clear that the values wouldn't change once we saw them. But I couldn't\n>>>> detect any difference. At most I think that would let us hoist the \"if\"\n>>>> out of the loop, but gcc still seems unwilling to expand the memcmp when\n>>>> there are other branches.\n>>>>\n>>>> I think if that's the thing we want to have happen, we really do need to\n>>>> just write it out on that branch rather than saying \"memcmp\".\n>>> This reminds me of an old discussion about memcpy() vs doing explicit\n>>> compare loop with lots of performance measurements..\n>> Ah found it. Not sure if it is still relevant in light of multiple hash support\n>>\n>> https://public-inbox.org/git/20110427225114.GA16765@elte.hu/\n> Yes, that was what I meant. We actually did switch to that hand-rolled\n> loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\n> memcmp instead of open-coded loop, 2017-08-09).\n\nLooking at that commit, I'm surprised the old logic was just a for loop, instead of a word-based approach, such as the following:\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..5e5819ad49 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1021,9 +1021,41 @@ extern int find_unique_abbrev_r(char *hex, const \nstruct object_id *oid, int len)\n  extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n  extern const struct object_id null_oid;\n\n+static inline int word_cmp_32(uint32_t a, uint32_t b)\n+{\n+       return memcmp(&a, &b, sizeof(uint32_t));\n+}\n+\n+static inline int word_cmp_64(uint64_t a, uint64_t b)\n+{\n+       return memcmp(&a, &b, sizeof(uint64_t));\n+}\n+\n+struct object_id_20 {\n+       uint64_t data0;\n+       uint64_t data1;\n+       uint32_t data2;\n+};\n+\n  static inline int hashcmp(const unsigned char *sha1, const unsigned \nchar *sha2)\n  {\n-       return memcmp(sha1, sha2, the_hash_algo->rawsz);\n+       if (the_hash_algo->rawsz == 20) {\n+               struct object_id_20 *obj1 = (struct object_id_20 *)sha1;\n+               struct object_id_20 *obj2 = (struct object_id_20 *)sha2;\n+\n+               if (obj1->data0 == obj2->data0) {\n+                       if (obj1->data1 == obj2->data1) {\n+                               if (obj1->data2 == obj2->data2) {\n+                                       return 0;\n+                               }\n+                               return word_cmp_32(obj1->data2, \nobj2->data2);\n+                       }\n+                       return word_cmp_64(obj1->data1, obj2->data1);\n+               }\n+               return word_cmp_64(obj1->data0, obj2->data0);\n+       }\n+\n+       assert(0);\n  }\n\n  static inline int oidcmp(const struct object_id *oid1, const struct \nobject_id *oid2)\n\n\n"},{"id":"356283","messageId":"CACsJy8By49nz6-q78GuMYP8ssrjhWYyhYrJcL8eWpi4-mZzzxA@mail.gmail.com","threadId":"49173","inReplyTo":"7ea416cf-b043-1274-e161-85a8780b8e1c@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-22T16:58:44Z","receivedAt":"2018-08-22T16:59:13Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2018 at 6:49 PM Derrick Stolee wrote:\n>\n> On 8/22/2018 12:26 PM, Jeff King wrote:\n> > On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote:\n> >\n> >> On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen wrote:\n> >>> On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n> >>>> On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n> >>>>\n> >>>>> The other thing I was going to recommend (and I'll try to test this out\n> >>>>> myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n> >>>>> volatile variable, since it is being referenced through a pointer. Perhaps\n> >>>>> storing the value locally and then casing on it would help?\n> >>>> I tried various sprinkling of \"const\" around the declarations to make it\n> >>>> clear that the values wouldn't change once we saw them. But I couldn't\n> >>>> detect any difference. At most I think that would let us hoist the \"if\"\n> >>>> out of the loop, but gcc still seems unwilling to expand the memcmp when\n> >>>> there are other branches.\n> >>>>\n> >>>> I think if that's the thing we want to have happen, we really do need to\n> >>>> just write it out on that branch rather than saying \"memcmp\".\n> >>> This reminds me of an old discussion about memcpy() vs doing explicit\n> >>> compare loop with lots of performance measurements..\n> >> Ah found it. Not sure if it is still relevant in light of multiple hash support\n> >>\n> >> https://public-inbox.org/git/20110427225114.GA16765@elte.hu/\n> > Yes, that was what I meant. We actually did switch to that hand-rolled\n> > loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\n> > memcmp instead of open-coded loop, 2017-08-09).\n>\n> Looking at that commit, I'm surprised the old logic was just a for loop, instead of a word-based approach, such as the following:\n\nMight work on x86 but it breaks on cpu architectures with stricter\nalignment. I don't think we have a guarantee that object_id is always\n8 byte aligned.\n\n>\n> diff --git a/cache.h b/cache.h\n> index b1fd3d58ab..5e5819ad49 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1021,9 +1021,41 @@ extern int find_unique_abbrev_r(char *hex, const\n> struct object_id *oid, int len)\n> extern const unsigned char null_sha1[GIT_MAX_RAWSZ];\n> extern const struct object_id null_oid;\n>\n> +static inline int word_cmp_32(uint32_t a, uint32_t b)\n> +{\n> + return memcmp(&a, &b, sizeof(uint32_t));\n> +}\n> +\n> +static inline int word_cmp_64(uint64_t a, uint64_t b)\n> +{\n> + return memcmp(&a, &b, sizeof(uint64_t));\n> +}\n> +\n> +struct object_id_20 {\n> + uint64_t data0;\n> + uint64_t data1;\n> + uint32_t data2;\n> +};\n> +\n> static inline int hashcmp(const unsigned char *sha1, const unsigned\n> char *sha2)\n> {\n> - return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> + if (the_hash_algo->rawsz == 20) {\n> + struct object_id_20 *obj1 = (struct object_id_20 *)sha1;\n> + struct object_id_20 *obj2 = (struct object_id_20 *)sha2;\n> +\n> + if (obj1->data0 == obj2->data0) {\n> + if (obj1->data1 == obj2->data1) {\n> + if (obj1->data2 == obj2->data2) {\n> + return 0;\n> + }\n> + return word_cmp_32(obj1->data2,\n> obj2->data2);\n> + }\n> + return word_cmp_64(obj1->data1, obj2->data1);\n> + }\n> + return word_cmp_64(obj1->data0, obj2->data0);\n> + }\n> +\n> + assert(0);\n> }\n>\n> static inline int oidcmp(const struct object_id *oid1, const struct\n> object_id *oid2)\n>\n>\n\n\n-- \nDuy\n"},{"id":"356284","messageId":"20180822165923.GA14878@sigill.intra.peff.net","threadId":"49173","inReplyTo":"7ea416cf-b043-1274-e161-85a8780b8e1c@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T16:59:23Z","receivedAt":"2018-08-22T16:59:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 12:49:34PM -0400, Derrick Stolee wrote:\n\n> > Yes, that was what I meant. We actually did switch to that hand-rolled\n> > loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\n> > memcmp instead of open-coded loop, 2017-08-09).\n> \n> Looking at that commit, I'm surprised the old logic was just a for\n> loop, instead of a word-based approach, such as the following:\n> [...]\n> +struct object_id_20 {\n> +       uint64_t data0;\n> +       uint64_t data1;\n> +       uint32_t data2;\n> +};\n> +\n>  static inline int hashcmp(const unsigned char *sha1, const unsigned char\n> *sha2)\n>  {\n> -       return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> +       if (the_hash_algo->rawsz == 20) {\n> +               struct object_id_20 *obj1 = (struct object_id_20 *)sha1;\n> +               struct object_id_20 *obj2 = (struct object_id_20 *)sha2;\n\nI wonder if you're potentially running afoul of alignment requirements\nhere.\n\n-Peff\n"},{"id":"356285","messageId":"xmqqtvnmmj0c.fsf@gitster-ct.c.googlers.com","threadId":"49173","inReplyTo":"20180822165923.GA14878@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-22T17:02:59Z","receivedAt":"2018-08-22T17:03:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 22, 2018 at 12:49:34PM -0400, Derrick Stolee wrote:\n>\n>> > Yes, that was what I meant. We actually did switch to that hand-rolled\n>> > loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\n>> > memcmp instead of open-coded loop, 2017-08-09).\n>> \n>> Looking at that commit, I'm surprised the old logic was just a for\n>> loop, instead of a word-based approach, such as the following:\n>> [...]\n>> +struct object_id_20 {\n>> +       uint64_t data0;\n>> +       uint64_t data1;\n>> +       uint32_t data2;\n>> +};\n>> +\n>>  static inline int hashcmp(const unsigned char *sha1, const unsigned char\n>> *sha2)\n>>  {\n>> -       return memcmp(sha1, sha2, the_hash_algo->rawsz);\n>> +       if (the_hash_algo->rawsz == 20) {\n>> +               struct object_id_20 *obj1 = (struct object_id_20 *)sha1;\n>> +               struct object_id_20 *obj2 = (struct object_id_20 *)sha2;\n>\n> I wonder if you're potentially running afoul of alignment requirements\n> here.\n\nYup, and I think that all was discussed in that old thread ;-)\n"},{"id":"356286","messageId":"387295e3-3112-ed09-d988-3d8156e3c7ce@gmail.com","threadId":"49173","inReplyTo":"CACsJy8By49nz6-q78GuMYP8ssrjhWYyhYrJcL8eWpi4-mZzzxA@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T17:04:08Z","receivedAt":"2018-08-22T17:04:14Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/22/2018 12:58 PM, Duy Nguyen wrote:\n> On Wed, Aug 22, 2018 at 6:49 PM Derrick Stolee wrote:\n>> On 8/22/2018 12:26 PM, Jeff King wrote:\n>>> On Wed, Aug 22, 2018 at 06:14:24PM +0200, Duy Nguyen wrote:\n>>>\n>>>> On Wed, Aug 22, 2018 at 6:08 PM Duy Nguyen wrote:\n>>>>> On Wed, Aug 22, 2018 at 6:03 PM Jeff King wrote:\n>>>>>> On Wed, Aug 22, 2018 at 07:14:42AM -0400, Derrick Stolee wrote:\n>>>>>>\n>>>>>>> The other thing I was going to recommend (and I'll try to test this out\n>>>>>>> myself later) is to see if 'the_hash_algo->rawsz' is being treated as a\n>>>>>>> volatile variable, since it is being referenced through a pointer. Perhaps\n>>>>>>> storing the value locally and then casing on it would help?\n>>>>>> I tried various sprinkling of \"const\" around the declarations to make it\n>>>>>> clear that the values wouldn't change once we saw them. But I couldn't\n>>>>>> detect any difference. At most I think that would let us hoist the \"if\"\n>>>>>> out of the loop, but gcc still seems unwilling to expand the memcmp when\n>>>>>> there are other branches.\n>>>>>>\n>>>>>> I think if that's the thing we want to have happen, we really do need to\n>>>>>> just write it out on that branch rather than saying \"memcmp\".\n>>>>> This reminds me of an old discussion about memcpy() vs doing explicit\n>>>>> compare loop with lots of performance measurements..\n>>>> Ah found it. Not sure if it is still relevant in light of multiple hash support\n>>>>\n>>>> https://public-inbox.org/git/20110427225114.GA16765@elte.hu/\n>>> Yes, that was what I meant. We actually did switch to that hand-rolled\n>>> loop, but later we went back to memcmp in 0b006014c8 (hashcmp: use\n>>> memcmp instead of open-coded loop, 2017-08-09).\n>> Looking at that commit, I'm surprised the old logic was just a for loop, instead of a word-based approach, such as the following:\n> Might work on x86 but it breaks on cpu architectures with stricter\n> alignment. I don't think we have a guarantee that object_id is always\n> 8 byte aligned.\nYou (and Peff) are probably correct here, which is unfortunate. I'm not \nfamiliar with alignment constraints, but assume that such a word-based \napproach is best.\n\nThanks,\n-Stolee\n\n"},{"id":"356330","messageId":"20180823012343.GB92374@aiede.svl.corp.google.com","threadId":"49173","inReplyTo":"20180822152306.GC32630@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-23T01:23:43Z","receivedAt":"2018-08-23T01:23:48Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n>> On Tue, 2018-08-21 at 23:03 -0400, Jeff King wrote:\n\n>>> static inline int hashcmp(const unsigned char *sha1, const unsigned\n>>> char *sha2)\n>>> {\n>>> + assert(the_hash_algo->rawsz == 20);\n>>> return memcmp(sha1, sha2, the_hash_algo->rawsz);\n>>> }\n[...]\n> The bigger questions are:\n>\n> - are we OK with such an assertion; and\n>\n> - does the assertion still give us the desired behavior when we add in\n> a branch for rawsz==32?\n>\n> And I think the answers for those are both \"probably not\".\n\nAt this point in the release process, I think the answer to the first\nquestion is a pretty clear \"yes\".\n\nA ~10% increase in latency of some operations is quite significant, in\nexchange for no user benefit yet. We can continue to try to figure\nout how to convince compilers to generate good code for this (and\nthat's useful), but in the meantime we should also do the simple thing\nto avoid the regression for users.\n\nThanks,\nJonathan\n"},{"id":"356334","messageId":"20180823021618.GA12052@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180823012343.GB92374@aiede.svl.corp.google.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T02:16:18Z","receivedAt":"2018-08-23T02:16:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 06:23:43PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> >> On Tue, 2018-08-21 at 23:03 -0400, Jeff King wrote:\n> \n> >>> static inline int hashcmp(const unsigned char *sha1, const unsigned\n> >>> char *sha2)\n> >>> {\n> >>> + assert(the_hash_algo->rawsz == 20);\n> >>> return memcmp(sha1, sha2, the_hash_algo->rawsz);\n> >>> }\n> [...]\n> > The bigger questions are:\n> >\n> > - are we OK with such an assertion; and\n> >\n> > - does the assertion still give us the desired behavior when we add in\n> > a branch for rawsz==32?\n> >\n> > And I think the answers for those are both \"probably not\".\n> \n> At this point in the release process, I think the answer to the first\n> question is a pretty clear \"yes\".\n> \n> A ~10% increase in latency of some operations is quite significant, in\n> exchange for no user benefit yet. We can continue to try to figure\n> out how to convince compilers to generate good code for this (and\n> that's useful), but in the meantime we should also do the simple thing\n> to avoid the regression for users.\n\nFWIW, it's not 10%. The best I measured was ~4% on a very\nhashcmp-limited operation, and I suspect even that may be highly\ndependent on the compiler. We might be able to improve more by\nsprinkling more asserts around, but there are 75 mentions of\nthe_hash_algo->rawsz. I wouldn't want to an assert at each one.\n\nI don't mind doing one or a handful of these asserts as part of v2.19 if\nwe want to try to reclaim those few percent. But I suspect the very\nfirst commit in any further hash-transition work is just going to be to\nrip them all out.\n\n-Peff\n"},{"id":"356335","messageId":"20180823022756.GF92374@aiede.svl.corp.google.com","threadId":"49173","inReplyTo":"20180823021618.GA12052@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-23T02:27:56Z","receivedAt":"2018-08-23T02:28:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> FWIW, it's not 10%. The best I measured was ~4% on a very\n> hashcmp-limited operation, and I suspect even that may be highly\n> dependent on the compiler. We might be able to improve more by\n> sprinkling more asserts around, but there are 75 mentions of\n> the_hash_algo->rawsz. I wouldn't want to an assert at each one.\n>\n> I don't mind doing one or a handful of these asserts as part of v2.19 if\n> we want to try to reclaim those few percent. But I suspect the very\n> first commit in any further hash-transition work is just going to be to\n> rip them all out.\n\nI was thinking just hashcmp and hashcpy.\n\nIdeally such a change would come with a performance test to help the\nperson writing that very first commit. Except we already have\nperformance tests that capture this. ;-)\n\nFor further hash-transition work, I agree someone may want to revert\nthis, and I don't mind such a revert appearing right away in \"next\".\nAnd it's possible that we might have to do the equivalent of manual\ntemplate expansion to recover the performance in some\nperformance-sensitive areas. Maybe we can get the compiler to\ncooperate with us in that and maybe we can't. That's okay with me.\n\nAnyway, I'll resend your patch with a commit message added some time\nthis evening.\n\nThanks,\nJonathan\n"},{"id":"356341","messageId":"20180823034707.GD535143@genre.crustytoothpaste.net","threadId":"49173","inReplyTo":"20180823021618.GA12052@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-23T03:47:07Z","receivedAt":"2018-08-23T03:47:53Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Aug 22, 2018 at 10:16:18PM -0400, Jeff King wrote:\n> FWIW, it's not 10%. The best I measured was ~4% on a very\n> hashcmp-limited operation, and I suspect even that may be highly\n> dependent on the compiler. We might be able to improve more by\n> sprinkling more asserts around, but there are 75 mentions of\n> the_hash_algo->rawsz. I wouldn't want to an assert at each one.\n> \n> I don't mind doing one or a handful of these asserts as part of v2.19 if\n> we want to try to reclaim those few percent. But I suspect the very\n> first commit in any further hash-transition work is just going to be to\n> rip them all out.\n\nI expect that's going to be the case as well. I have patches that\nwire up actual SHA-256 support in my hash-impl branch.\n\nHowever, having said that, I'm happy to defer to whatever everyone else\nthinks is best for 2.19. The assert solution would be fine with me in\nthis situation, and if we need to pull it out in the future, that's okay\nwith me.\n\nI don't really have a strong opinion on this either way, so if someone\nelse does, please say so. I have somewhat more limited availability\nover the next couple days, as I'm travelling on business, but I'm happy\nto review a patch (and it seems like Peff has one minus the actual\ncommit message).\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"356344","messageId":"20180823050224.GA318@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180823022756.GF92374@aiede.svl.corp.google.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T05:02:25Z","receivedAt":"2018-08-23T05:02:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 07:27:56PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > FWIW, it's not 10%. The best I measured was ~4% on a very\n> > hashcmp-limited operation, and I suspect even that may be highly\n> > dependent on the compiler. We might be able to improve more by\n> > sprinkling more asserts around, but there are 75 mentions of\n> > the_hash_algo->rawsz. I wouldn't want to an assert at each one.\n> >\n> > I don't mind doing one or a handful of these asserts as part of v2.19 if\n> > we want to try to reclaim those few percent. But I suspect the very\n> > first commit in any further hash-transition work is just going to be to\n> > rip them all out.\n> \n> I was thinking just hashcmp and hashcpy.\n> \n> Ideally such a change would come with a performance test to help the\n> person writing that very first commit. Except we already have\n> performance tests that capture this. ;-)\n> \n> For further hash-transition work, I agree someone may want to revert\n> this, and I don't mind such a revert appearing right away in \"next\".\n> And it's possible that we might have to do the equivalent of manual\n> template expansion to recover the performance in some\n> performance-sensitive areas. Maybe we can get the compiler to\n> cooperate with us in that and maybe we can't. That's okay with me.\n> \n> Anyway, I'll resend your patch with a commit message added some time\n> this evening.\n\nHere's the patch. For some reason my numbers aren't quite as large as\nthey were yesterday (I was very careful to keep the system unloaded\ntoday, whereas yesterday I was doing a few other things, so perhaps that\nis the difference).\n\n-- >8 --\nSubject: [PATCH] hashcmp: assert constant hash size\n\nPrior to 509f6f62a4 (cache: update object ID functions for\nthe_hash_algo, 2018-07-16), hashcmp() called memcmp() with a\nconstant size of 20 bytes. Some compilers were able to turn\nthat into a few quad-word comparisons, which is faster than\nactually calling memcmp().\n\nIn 509f6f62a4, we started using the_hash_algo->rawsz\ninstead. Even though this will always be 20, the compiler\ndoesn't know that while inlining hashcmp() and ends up just\ngenerating a call to memcmp().\n\nEventually we'll have to deal with multiple hash sizes, but\nfor the upcoming v2.19, we can restore some of the original\nperformance by asserting on the size. That gives the\ncompiler enough information to know that the memcmp will\nalways be called with a length of 20, and it performs the\nsame optimization.\n\nHere are numbers for p0001.2 run against linux.git on a few\nversions. This is using -O2 with gcc 8.2.0.\n\n Test v2.18.0 v2.19.0-rc0 HEAD\n ------------------------------------------------------------------------------\n 0001.2: 34.24(33.81+0.43) 34.83(34.42+0.40) +1.7% 33.90(33.47+0.42) -1.0%\n\nYou can see that v2.19 is a little slower than v2.18. This\ncommit ended up slightly faster than v2.18, but there's a\nfair bit of run-to-run noise (the generated code in the two\ncases is basically the same). This patch does seem to be\nconsistently 1-2% faster than v2.19.\n\nI tried changing hashcpy(), which was also touched by\n509f6f62a4, in the same way, but couldn't measure any\nspeedup. Which makes sense, at least for this workload. A\ntraversal of the whole commit graph requires looking up\nevery entry of every tree via lookup_object(). That's many\nmultiples of the numbers of objects in the repository (most\nof the lookups just return \"yes, we already saw that\nobject\").\n\nReported-by: Derrick Stolee \nSigned-off-by: Jeff King \n---\n cache.h | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex b1fd3d58ab..4d014541ab 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1023,6 +1023,16 @@ extern const struct object_id null_oid;\n \n static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n {\n+\t/*\n+\t * This is a temporary optimization hack. By asserting the size here,\n+\t * we let the compiler know that it's always going to be 20, which lets\n+\t * it turn this fixed-size memcmp into a few inline instructions.\n+\t *\n+\t * This will need to be extended or ripped out when we learn about\n+\t * hashes of different sizes.\n+\t */\n+\tif (the_hash_algo->rawsz != 20)\n+\t\tBUG(\"hash size not yet supported by hashcmp\");\n \treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n }\n \n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356345","messageId":"20180823050418.GB318@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180823034707.GD535143@genre.crustytoothpaste.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T05:04:19Z","receivedAt":"2018-08-23T05:04:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 03:47:07AM +0000, brian m. carlson wrote:\n\n> I expect that's going to be the case as well. I have patches that\n> wire up actual SHA-256 support in my hash-impl branch.\n> \n> However, having said that, I'm happy to defer to whatever everyone else\n> thinks is best for 2.19. The assert solution would be fine with me in\n> this situation, and if we need to pull it out in the future, that's okay\n> with me.\n> \n> I don't really have a strong opinion on this either way, so if someone\n> else does, please say so. I have somewhat more limited availability\n> over the next couple days, as I'm travelling on business, but I'm happy\n> to review a patch (and it seems like Peff has one minus the actual\n> commit message).\n\nI just posted the patch elsewhere in the thread. I think you can safely\nignore the rest of it if you are otherwise occupied. Even if v2.19 ships\nwithout some mitigation, I don't know that it's all that big a deal,\ngiven the numbers I generated (which for some reason are less dramatic\nthan Stolee's).\n\n-Peff\n"},{"id":"356346","messageId":"20180823050948.GE535143@genre.crustytoothpaste.net","threadId":"49173","inReplyTo":"20180823050224.GA318@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-23T05:09:49Z","receivedAt":"2018-08-23T05:09:58Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Aug 23, 2018 at 01:02:25AM -0400, Jeff King wrote:\n> Here's the patch. For some reason my numbers aren't quite as large as\n> they were yesterday (I was very careful to keep the system unloaded\n> today, whereas yesterday I was doing a few other things, so perhaps that\n> is the difference).\n\nThis looks sane to me. Thanks for writing this up.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"356347","messageId":"20180823051027.GA160081@aiede.svl.corp.google.com","threadId":"49173","inReplyTo":"20180823050224.GA318@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-23T05:10:27Z","receivedAt":"2018-08-23T05:10:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Here's the patch. For some reason my numbers aren't quite as large as\n> they were yesterday (I was very careful to keep the system unloaded\n> today, whereas yesterday I was doing a few other things, so perhaps that\n> is the difference).\n>\n> -- >8 --\n> Subject: [PATCH] hashcmp: assert constant hash size\n>\n> Prior to 509f6f62a4 (cache: update object ID functions for\n> the_hash_algo, 2018-07-16), hashcmp() called memcmp() with a\n> constant size of 20 bytes. Some compilers were able to turn\n> that into a few quad-word comparisons, which is faster than\n> actually calling memcmp().\n>\n> In 509f6f62a4, we started using the_hash_algo->rawsz\n> instead. Even though this will always be 20, the compiler\n> doesn't know that while inlining hashcmp() and ends up just\n> generating a call to memcmp().\n>\n> Eventually we'll have to deal with multiple hash sizes, but\n> for the upcoming v2.19, we can restore some of the original\n> performance by asserting on the size. That gives the\n> compiler enough information to know that the memcmp will\n> always be called with a length of 20, and it performs the\n> same optimization.\n>\n> Here are numbers for p0001.2 run against linux.git on a few\n> versions. This is using -O2 with gcc 8.2.0.\n>\n> Test v2.18.0 v2.19.0-rc0 HEAD\n> ------------------------------------------------------------------------------\n> 0001.2: 34.24(33.81+0.43) 34.83(34.42+0.40) +1.7% 33.90(33.47+0.42) -1.0%\n>\n> You can see that v2.19 is a little slower than v2.18. This\n> commit ended up slightly faster than v2.18, but there's a\n> fair bit of run-to-run noise (the generated code in the two\n> cases is basically the same). This patch does seem to be\n> consistently 1-2% faster than v2.19.\n>\n> I tried changing hashcpy(), which was also touched by\n> 509f6f62a4, in the same way, but couldn't measure any\n> speedup. Which makes sense, at least for this workload. A\n> traversal of the whole commit graph requires looking up\n> every entry of every tree via lookup_object(). That's many\n> multiples of the numbers of objects in the repository (most\n> of the lookups just return \"yes, we already saw that\n> object\").\n>\n> Reported-by: Derrick Stolee \n> Signed-off-by: Jeff King \n> ---\n> cache.h | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n\nReviewed-by: Jonathan Nieder \n\nVerified using \"make object.s\" that the memcmp call goes away. Thank\nyou.\n"},{"id":"356355","messageId":"f854aba0-6d28-7f2b-aad2-858983c4af36@gmail.com","threadId":"49173","inReplyTo":"20180823050418.GB318@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-23T10:26:58Z","receivedAt":"2018-08-23T10:27:05Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2018 1:04 AM, Jeff King wrote:\n> On Thu, Aug 23, 2018 at 03:47:07AM +0000, brian m. carlson wrote:\n>\n>> I expect that's going to be the case as well. I have patches that\n>> wire up actual SHA-256 support in my hash-impl branch.\n>>\n>> However, having said that, I'm happy to defer to whatever everyone else\n>> thinks is best for 2.19. The assert solution would be fine with me in\n>> this situation, and if we need to pull it out in the future, that's okay\n>> with me.\n>>\n>> I don't really have a strong opinion on this either way, so if someone\n>> else does, please say so. I have somewhat more limited availability\n>> over the next couple days, as I'm travelling on business, but I'm happy\n>> to review a patch (and it seems like Peff has one minus the actual\n>> commit message).\n> I just posted the patch elsewhere in the thread.\n\nThank you for that!\n\n> I think you can safely\n> ignore the rest of it if you are otherwise occupied. Even if v2.19 ships\n> without some mitigation, I don't know that it's all that big a deal,\n> given the numbers I generated (which for some reason are less dramatic\n> than Stolee's).\nMy numbers may be more dramatic because my Linux environment is a \nvirtual machine.\n\nI was thinking that having a mitigation for 2.19 is best, and then we \ncan focus as part of the 2.20 cycle how we can properly avoid this cost, \nespecially when 32 is a valid option.\n\nAround the time that my proposed approaches were getting vetoed for \nalignment issues, I figured I was out of my depth here. I reached out to \nDaniel Lemire (of EWAH bitmap fame) on Twitter [1]. His blog is full of \nposts of word-based approaches to different problems, so I thought he \nmight know something off the top of his head that would be applicable. \nHis conclusion (after looking only a short time) was to take a 'hasheq' \napproach [2] like Peff suggested [3]. Since that requires auditing all \ncallers of hashcmp to see if hasheq is appropriate, it is not a good \nsolution for 2.19 but (in my opinion) should be evaluated as part of the \n2.20 cycle.\n\nOf course, if someone with knowledge of word-alignment issues across the \nplatforms we support knows how to enforce an alignment for object_id, \nthen something word-based like [4] could be reconsidered.\n\nThanks, everyone!\n-Stolee\n\n[1] https://twitter.com/stolee/status/1032312965754748930\n\n[2] \nhttps://lemire.me/blog/2018/08/22/avoid-lexicographical-comparisons-when-testing-for-string-equality/\n\n[3] \nhttps://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/\n\n[4] \nhttps://public-inbox.org/git/7ea416cf-b043-1274-e161-85a8780b8e1c@gmail.com/\n"},{"id":"356359","messageId":"xmqqefepkytk.fsf@gitster-ct.c.googlers.com","threadId":"49173","inReplyTo":"f854aba0-6d28-7f2b-aad2-858983c4af36@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-23T13:16:39Z","receivedAt":"2018-08-23T13:16:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> I was thinking that having a mitigation for 2.19 is best, and then we\n> can focus as part of the 2.20 cycle how we can properly avoid this\n> cost, especially when 32 is a valid option.\n> ...\n> ... to\n> take a 'hasheq' approach [2] like Peff suggested [3]. Since that\n> requires auditing all callers of hashcmp to see if hasheq is\n> appropriate, it is not a good solution for 2.19 but (in my opinion)\n> should be evaluated as part of the 2.20 cycle.\n\nThanks for thoughtful comments. I think it makes sense to go with\nthe \"tell compiler hashcmp() currently is only about 20-byte array\"\nfor now, as it is trivial to see why it cannot break things.\n\nDuring 2.20 cycle, we may find out that hasheq() abstraction\nperforms better than hashcmp() with multiple lengths, and end up\ndoing that \"audit and replace to use hasheq()\". But as you said,\nit probably isnot a good idea to rush it in this cycle.\n"},{"id":"356360","messageId":"xmqqa7pdkyn9.fsf@gitster-ct.c.googlers.com","threadId":"49173","inReplyTo":"20180823050224.GA318@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-23T13:20:26Z","receivedAt":"2018-08-23T13:20:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here are numbers for p0001.2 run against linux.git on a few\n> versions. This is using -O2 with gcc 8.2.0.\n>\n> Test v2.18.0 v2.19.0-rc0 HEAD\n> ------------------------------------------------------------------------------\n> 0001.2: 34.24(33.81+0.43) 34.83(34.42+0.40) +1.7% 33.90(33.47+0.42) -1.0%\n\nI see what you did to the formatting here, which is a topic of\nanother thread ;-).\n\nThanks, as Derrick also noted, I agree this is an appropriate\nworkaround for the upcoming release and we may want to explore\nhasheq() and other solutions as part of the effort during the next\ncycle (which can start now if people are bored---after all working\non the codebase is the best way to hunt for recent bugs).\n\nThanks, will queue.\n\n> diff --git a/cache.h b/cache.h\n> index b1fd3d58ab..4d014541ab 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1023,6 +1023,16 @@ extern const struct object_id null_oid;\n> \n> static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)\n> {\n> +\t/*\n> +\t * This is a temporary optimization hack. By asserting the size here,\n> +\t * we let the compiler know that it's always going to be 20, which lets\n> +\t * it turn this fixed-size memcmp into a few inline instructions.\n> +\t *\n> +\t * This will need to be extended or ripped out when we learn about\n> +\t * hashes of different sizes.\n> +\t */\n> +\tif (the_hash_algo->rawsz != 20)\n> +\t\tBUG(\"hash size not yet supported by hashcmp\");\n> \treturn memcmp(sha1, sha2, the_hash_algo->rawsz);\n> }\n"},{"id":"356381","messageId":"20180823161451.GB29579@sigill.intra.peff.net","threadId":"49173","inReplyTo":"f854aba0-6d28-7f2b-aad2-858983c4af36@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T16:14:51Z","receivedAt":"2018-08-23T16:14:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 06:26:58AM -0400, Derrick Stolee wrote:\n\n> Around the time that my proposed approaches were getting vetoed for\n> alignment issues, I figured I was out of my depth here. I reached out to\n> Daniel Lemire (of EWAH bitmap fame) on Twitter [1]. His blog is full of\n> posts of word-based approaches to different problems, so I thought he might\n> know something off the top of his head that would be applicable. His\n> conclusion (after looking only a short time) was to take a 'hasheq' approach\n> [2] like Peff suggested [3]. Since that requires auditing all callers of\n> hashcmp to see if hasheq is appropriate, it is not a good solution for 2.19\n> but (in my opinion) should be evaluated as part of the 2.20 cycle.\n\nI think that audit isn't actually too bad (but definitely not something\nwe should do for v2.19). The cocci patch I showed earlier hits most of\nthem. It misses the negated ones (e.g., \"if (oidcmp(...))\"). I'm not\nsure if there's a way to ask coccinelle only for oidcmp()\nused in a boolean context.\n\nJust skimming the grep output, it's basically every call except the ones\nwe use for binary search.\n\n-Peff\n"},{"id":"356383","messageId":"20180823163130.GC29579@sigill.intra.peff.net","threadId":"49173","inReplyTo":"xmqqa7pdkyn9.fsf@gitster-ct.c.googlers.com","subject":"wide t/perf output, was Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T16:31:30Z","receivedAt":"2018-08-23T16:31:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 06:20:26AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Here are numbers for p0001.2 run against linux.git on a few\n> > versions. This is using -O2 with gcc 8.2.0.\n> >\n> > Test v2.18.0 v2.19.0-rc0 HEAD\n> > ------------------------------------------------------------------------------\n> > 0001.2: 34.24(33.81+0.43) 34.83(34.42+0.40) +1.7% 33.90(33.47+0.42) -1.0%\n> \n> I see what you did to the formatting here, which is a topic of\n> another thread ;-).\n\nDo you happen to have a link? I missed that one, and digging turned up\nnothing.\n\nA while ago I wrote the patch below. I don't recall why I never sent it\nin (and it doesn't apply cleanly these days, though I'm sure it could be\nforward-ported).\n\n-- >8 --\nDate: Wed, 20 Jan 2016 23:54:14 -0500\nSubject: [PATCH] t/perf: add \"tall\" output format\n\nWhen aggregating results, we usually show a list of tests,\none per line, with the tested revisions in columns across.\nLike:\n\n $ ./aggregate.perl 348d4f2^ 348d4f2 p7000-filter-branch.sh\n Test 348d4f2^ 348d4f2\n -------------------------------------------------------------------\n 7000.2: noop filter 295.32(269.61+14.36) 7.92(0.85+0.72) -97.3%\n\nThis is useful if you have a lot of tests to show, but few\nrevisions; you're effectively comparing the two items on\neach line. But sometimes you have the opposite: few tests,\nbut a large number of revisions. In this case, the lines\nget very long, and it's hard to compare values.\n\nThis patch introduces a \"tall\" format that shows the same\ndata in a more vertical manner:\n\n $ ./aggregate.perl --tall \\\n 348d4f2^ 348d4f2 \\\n jk/filter-branch-empty^ \\\n jk/filter-branch-empty \\\n p7000-filter-branch.sh\n Test: p7000-filter-branch.2\n 348d4f2^ 295.32(269.61+14.36)\n 348d4f2 7.92(0.85+0.72) -97.3%\n jk/filter-branch-empty^ 9.37(0.87+0.80) -96.8%\n jk/filter-branch-empty 7.71(0.92+0.62) -97.4%\n\nSigned-off-by: Jeff King \n---\n t/perf/aggregate.perl | 124 ++++++++++++++++++++++++++++++------------\n 1 file changed, 88 insertions(+), 36 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex e401208488..d108a02ccd 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -17,29 +17,41 @@ sub get_times {\n \treturn ($rt, $4, $5);\n }\n \n-sub format_times {\n+sub format_times_list {\n \tmy ($r, $u, $s, $firstr) = @_;\n \tif (!defined $r) {\n \t\treturn \"\";\n \t}\n \tmy $out = sprintf \"%.2f(%.2f+%.2f)\", $r, $u, $s;\n+\tmy $pct;\n \tif (defined $firstr) {\n \t\tif ($firstr > 0) {\n-\t\t\t$out .= sprintf \" %+.1f%%\", 100.0*($r-$firstr)/$firstr;\n+\t\t\t$pct = sprintf \"%+.1f%%\", 100.0*($r-$firstr)/$firstr;\n \t\t} elsif ($r == 0) {\n-\t\t\t$out .= \" =\";\n+\t\t\t$pct = \"=\";\n \t\t} else {\n-\t\t\t$out .= \" +inf\";\n+\t\t\t$pct = \"+inf\";\n \t\t}\n \t}\n-\treturn $out;\n+\treturn ($out, $pct);\n+}\n+\n+sub format_times {\n+\tmy ($times, $pct) = format_times_list(@_);\n+\treturn defined $pct ? \"$times $pct\" : $times;\n }\n \n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests);\n+my ($tall_format);\n while (scalar @ARGV) {\n \tmy $arg = $ARGV[0];\n \tmy $dir;\n \tlast if -f $arg or $arg eq \"--\";\n+\tif ($arg eq \"--tall\") {\n+\t\t$tall_format = 1;\n+\t\tshift @ARGV;\n+\t\tnext;\n+\t}\n \tif (! -d $arg) {\n \t\tmy $rev = Git::command_oneline(qw(rev-parse --verify), $arg);\n \t\t$dir = \"build/\".$rev;\n@@ -122,6 +134,11 @@ sub have_slash {\n \treturn 0;\n }\n \n+sub printable_dir {\n+\tmy ($d) = @_;\n+\treturn exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d};\n+}\n+\n my %newdirabbrevs = %dirabbrevs;\n while (!have_duplicate(values %newdirabbrevs)) {\n \t%dirabbrevs = %newdirabbrevs;\n@@ -132,44 +149,79 @@ sub have_slash {\n \t}\n }\n \n-my %times;\n-my @colwidth = ((0)x@dirs);\n-for my $i (0..$#dirs) {\n-\tmy $d = $dirs[$i];\n-\tmy $w = length (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});\n-\t$colwidth[$i] = $w if $w > $colwidth[$i];\n-}\n-for my $t (@subtests) {\n-\tmy $firstr;\n+binmode STDOUT, \":utf8\" or die \"PANIC on binmode: $!\";\n+\n+if (!$tall_format) {\n+\tmy %times;\n+\tmy @colwidth = ((0)x@dirs);\n \tfor my $i (0..$#dirs) {\n \t\tmy $d = $dirs[$i];\n-\t\t$times{$prefixes{$d}.$t} = [get_times(\"$resultsdir/$prefixes{$d}$t.times\")];\n-\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n-\t\tmy $w = length format_times($r,$u,$s,$firstr);\n+\t\tmy $w = length(printable_dir($d));\n \t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n-\t\t$firstr = $r unless defined $firstr;\n \t}\n-}\n-my $totalwidth = 3*@dirs+$descrlen;\n-$totalwidth += $_ for (@colwidth);\n-\n-binmode STDOUT, \":utf8\" or die \"PANIC on binmode: $!\";\n+\tfor my $t (@subtests) {\n+\t\tmy $firstr;\n+\t\tfor my $i (0..$#dirs) {\n+\t\t\tmy $d = $dirs[$i];\n+\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$resultsdir/$prefixes{$d}$t.times\")];\n+\t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n+\t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n+\t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\n+\t\t\t$firstr = $r unless defined $firstr;\n+\t\t}\n+\t}\n+\tmy $totalwidth = 3*@dirs+$descrlen;\n+\t$totalwidth += $_ for (@colwidth);\n \n-printf \"%-${descrlen}s\", \"Test\";\n-for my $i (0..$#dirs) {\n-\tmy $d = $dirs[$i];\n-\tprintf \" %-$colwidth[$i]s\", (exists $dirabbrevs{$d} ? $dirabbrevs{$d} : $dirnames{$d});\n-}\n-print \"\\n\";\n-print \"-\"x$totalwidth, \"\\n\";\n-for my $t (@subtests) {\n-\tprintf \"%-${descrlen}s\", $descrs{$t};\n-\tmy $firstr;\n+\tprintf \"%-${descrlen}s\", \"Test\";\n \tfor my $i (0..$#dirs) {\n \t\tmy $d = $dirs[$i];\n-\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n-\t\tprintf \" %-$colwidth[$i]s\", format_times($r,$u,$s,$firstr);\n-\t\t$firstr = $r unless defined $firstr;\n+\t\tprintf \" %-$colwidth[$i]s\", printable_dir($d);\n \t}\n \tprint \"\\n\";\n+\tprint \"-\"x$totalwidth, \"\\n\";\n+\tfor my $t (@subtests) {\n+\t\tprintf \"%-${descrlen}s\", $descrs{$t};\n+\t\tmy $firstr;\n+\t\tfor my $i (0..$#dirs) {\n+\t\t\tmy $d = $dirs[$i];\n+\t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n+\t\t\tprintf \" %-$colwidth[$i]s\", format_times($r,$u,$s,$firstr);\n+\t\t\t$firstr = $r unless defined $firstr;\n+\t\t}\n+\t\tprint \"\\n\";\n+\t}\n+} else {\n+\tmy $shown = 0;\n+\tfor my $t (@subtests) {\n+\t\tprint \"\\n\" if $shown++;\n+\t\tprint \"Test: $t\\n\";\n+\n+\t\tmy %times;\n+\t\tmy $firstr;\n+\t\tfor my $d (@dirs) {\n+\t\t\tmy ($r, $u, $s) = get_times(\"test-results/$prefixes{$d}$t.times\");\n+\t\t\t$times{$d} = [format_times_list($r, $u, $s, $firstr)];\n+\t\t\t$firstr = $r unless defined $firstr;\n+\t\t}\n+\n+\t\tmy $maxdirlen = 0;\n+\t\tmy $maxtimelen = 0;\n+\t\tfor my $d (@dirs) {\n+\t\t\tif (length($d) > $maxdirlen) {\n+\t\t\t\t$maxdirlen = length(printable_dir($d));\n+\t\t\t}\n+\t\t\tif (length($times{$d}->[0]) > $maxtimelen) {\n+\t\t\t\t$maxtimelen = length($times{$d}->[0]);\n+\t\t\t}\n+\t\t}\n+\t\t$maxdirlen++;\n+\n+\t\tfor my $d (@dirs) {\n+\t\t\tprintf \"%-${maxdirlen}s\", printable_dir($d);\n+\t\t\tprintf \" %${maxtimelen}s\", $times{$d}->[0];\n+\t\t\tprint \" $times{$d}->[1]\" if defined $times{$d}->[1];\n+\t\t\tprint \"\\n\";\n+\t\t}\n+\t}\n }\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356393","messageId":"20180823185317.GA12534@sigill.intra.peff.net","threadId":"49173","inReplyTo":"f854aba0-6d28-7f2b-aad2-858983c4af36@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T18:53:17Z","receivedAt":"2018-08-23T18:53:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 06:26:58AM -0400, Derrick Stolee wrote:\n\n> > I think you can safely\n> > ignore the rest of it if you are otherwise occupied. Even if v2.19 ships\n> > without some mitigation, I don't know that it's all that big a deal,\n> > given the numbers I generated (which for some reason are less dramatic\n> > than Stolee's).\n> My numbers may be more dramatic because my Linux environment is a virtual\n> machine.\n\nIf you have a chance, can you run p0001 on my patch (compared to\n2.19-rc0, or to both v2.18 and v2.19-rc0)? It would be nice to double\ncheck that it really is fixing the problem you saw.\n\n-Peff\n"},{"id":"356407","messageId":"4b0168ee-4826-1f14-fc83-04c4cec18687@gmail.com","threadId":"49173","inReplyTo":"20180823185317.GA12534@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-23T20:59:27Z","receivedAt":"2018-08-23T20:59:34Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2018 2:53 PM, Jeff King wrote:\n> On Thu, Aug 23, 2018 at 06:26:58AM -0400, Derrick Stolee wrote:\n>\n>>> I think you can safely\n>>> ignore the rest of it if you are otherwise occupied. Even if v2.19 ships\n>>> without some mitigation, I don't know that it's all that big a deal,\n>>> given the numbers I generated (which for some reason are less dramatic\n>>> than Stolee's).\n>> My numbers may be more dramatic because my Linux environment is a virtual\n>> machine.\n> If you have a chance, can you run p0001 on my patch (compared to\n> 2.19-rc0, or to both v2.18 and v2.19-rc0)? It would be nice to double\n> check that it really is fixing the problem you saw.\n\nSure. Note: I had to create a new Linux VM on a different machine \nbetween Tuesday and today, so the absolute numbers are different.\n\nUsing git/git:\n\nTest      v2.18.0           v2.19.0-rc0             HEAD\n-------------------------------------------------------------------------\n0001.2:   3.10(3.02+0.08)   3.27(3.17+0.09) +5.5% 3.14(3.02+0.11) +1.3%\n\n\nUsing torvalds/linux:\n\nTest     v2.18.0             v2.19.0-rc0               HEAD\n------------------------------------------------------------------------------\n0001.2:  56.08(45.91+1.50)   56.60(46.62+1.50) +0.9% 54.61(45.47+1.46) -2.6%\n\n\nNow here is where I get on my soapbox (and create a TODO for myself \nlater). I ran the above with GIT_PERF_REPEAT_COUNT=10, which intuitively \nsuggests that the results should be _more_ accurate than the default of \n3. However, I then remember that we only report the *minimum* time from \nall the runs, which is likely to select an outlier from the \ndistribution. To test this, I ran a few tests manually and found the \nvariation between runs to be larger than 3%.\n\nWhen I choose my own metrics for performance tests, I like to run at \nleast 10 runs, remove the largest AND smallest runs from the samples, \nand then take the average. I did this manually for 'git rev-list --all \n--objects' on git/git and got the following results:\n\nv2.18.0    v2.19.0-rc0   HEAD\n--------------------------------\n3.126 s    3.308 s       3.170 s\n\nFor full disclosure, here is a full table including all samples:\n\n|      | v2.18.0 | v2.19.0-rc0 | HEAD    |\n|------|---------|-------------|---------|\n|      | 4.58    | 3.302       | 3.239   |\n|      | 3.13    | 3.337       | 3.133   |\n|      | 3.213   | 3.291       | 3.159   |\n|      | 3.219   | 3.318       | 3.131   |\n|      | 3.077   | 3.302       | 3.163   |\n|      | 3.074   | 3.328       | 3.119   |\n|      | 3.022   | 3.277       | 3.125   |\n|      | 3.083   | 3.259       | 3.203   |\n|      | 3.057   | 3.311       | 3.223   |\n|      | 3.155   | 3.413       | 3.225   |\n| Max  | 4.58    | 3.413       | 3.239   |\n| Min  | 3.022   | 3.259       | 3.119   |\n| Avg* | 3.126   | 3.30825     | 3.17025 |\n\n(Note that the largest one was the first run, on v2.18.0, which is due \nto a cold disk.)\n\nI just kicked off a script that will run this test on the Linux repo \nwhile I drive home. I'll be able to report a similar table of data easily.\n\nMy TODO is to consider aggregating the data this way (or with a median) \ninstead of reporting the minimum.\n\nThanks,\n\n-Stolee\n\n"},{"id":"356423","messageId":"CA+P7+xqbt_BVi9+1-4=ha64LW_07dJB84F0gjKd9TRE1R-Ld7A@mail.gmail.com","threadId":"49173","inReplyTo":"20180823161451.GB29579@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-08-23T23:30:10Z","receivedAt":"2018-08-23T23:30:25Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 23, 2018 at 9:30 AM Jeff King wrote:\n> I think that audit isn't actually too bad (but definitely not something\n> we should do for v2.19). The cocci patch I showed earlier hits most of\n> them. It misses the negated ones (e.g., \"if (oidcmp(...))\"). I'm not\n> sure if there's a way to ask coccinelle only for oidcmp()\n> used in a boolean context.\n>\n\nYou can look for explicitly \"if (oidcmp(...))\" though. I don't know if\nyou can catch *any* use which degrades to boolean outside of an if\nstatement, but I wouldn't expect there to be too many of those?\n\nThanks,\nJake\n"},{"id":"356424","messageId":"20180823234049.GA3855@sigill.intra.peff.net","threadId":"49173","inReplyTo":"CA+P7+xqbt_BVi9+1-4=ha64LW_07dJB84F0gjKd9TRE1R-Ld7A@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T23:40:49Z","receivedAt":"2018-08-23T23:40:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 04:30:10PM -0700, Jacob Keller wrote:\n\n> On Thu, Aug 23, 2018 at 9:30 AM Jeff King wrote:\n> > I think that audit isn't actually too bad (but definitely not something\n> > we should do for v2.19). The cocci patch I showed earlier hits most of\n> > them. It misses the negated ones (e.g., \"if (oidcmp(...))\"). I'm not\n> > sure if there's a way to ask coccinelle only for oidcmp()\n> > used in a boolean context.\n> >\n> \n> You can look for explicitly \"if (oidcmp(...))\" though. I don't know if\n> you can catch *any* use which degrades to boolean outside of an if\n> statement, but I wouldn't expect there to be too many of those?\n\nYeah, that was my thought, too. And I've been trying this all afternoon\nwithout success. Why doesn't this work:\n\n @@\n expression a, b;\n @@\n - if (oidcmp(a, b))\n + if (!oideq(a, b))\n\nI get:\n\n Fatal error: exception Failure(\"minus: parse error: \\n = File\n \\\"contrib/coccinelle/oideq.cocci\\\", line 21, column 0, charpos =\n 221\\n around = '', whole content = \\n\")\n\nIf I do:\n\n - if (oidcmp(a, b)) { ... }\n\nthat seems to please the parser for the minus line. But I cannot include\nthe \"...\" on the plus line. Clearly the \"...\" part should be context,\nbut I can't seem to find the right syntax.\n\nFWIW, I do have patches adding hasheq() and converting all of the\n!oidcmp() cases. I may resort to hand-investigating each of the negated\nones, but I really feel like I should be able to do better with\ncoccinelle.\n\n-Peff\n"},{"id":"356425","messageId":"20180824000637.GA10847@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180823234049.GA3855@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T00:06:37Z","receivedAt":"2018-08-24T00:06:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 07:40:49PM -0400, Jeff King wrote:\n\n> > You can look for explicitly \"if (oidcmp(...))\" though. I don't know if\n> > you can catch *any* use which degrades to boolean outside of an if\n> > statement, but I wouldn't expect there to be too many of those?\n> \n> Yeah, that was my thought, too. And I've been trying this all afternoon\n> without success. Why doesn't this work:\n> \n> @@\n> expression a, b;\n> @@\n> - if (oidcmp(a, b))\n> + if (!oideq(a, b))\n> \n> I get:\n> \n> Fatal error: exception Failure(\"minus: parse error: \\n = File\n> \\\"contrib/coccinelle/oideq.cocci\\\", line 21, column 0, charpos =\n> 221\\n around = '', whole content = \\n\")\n> \n> If I do:\n> \n> - if (oidcmp(a, b)) { ... }\n> \n> that seems to please the parser for the minus line. But I cannot include\n> the \"...\" on the plus line. Clearly the \"...\" part should be context,\n> but I can't seem to find the right syntax.\n\nThis almost works:\n\n @@\n expression a, b;\n statement s;\n @@\n - if (oidcmp(a, b)) s\n + if (!oideq(a, b)) s\n\nIt generates this, for example:\n\ndiff -u -p a/bisect.c b/bisect.c\n--- a/bisect.c\n+++ b/bisect.c\n@@ -595,7 +595,7 @@ static struct commit_list *skip_away(str\n \n \tfor (i = 0; cur; cur = cur->next, i++) {\n \t\tif (i == index) {\n-\t\t\tif (oidcmp(&cur->item->object.oid, current_bad_oid))\n+\t\t\tif (!oideq(&cur->item->object.oid, current_bad_oid))\n \t\t\t\treturn cur;\n \t\t\tif (previous)\n \t\t\t\treturn previous;\n\nwhich is what we want. But it also generates this:\n\ndiff -u -p a/bundle.c b/bundle.c\n--- a/bundle.c\n+++ b/bundle.c\n@@ -369,25 +369,11 @@ static int write_bundle_refs(int bundle_\n \t\t * commit that is referenced by the tag, and not the tag\n \t\t * itself.\n \t\t */\n-\t\tif (oidcmp(&oid, &e->item->oid)) {\n-\t\t\t/*\n-\t\t\t * Is this the positive end of a range expressed\n-\t\t\t * in terms of a tag (e.g. v2.0 from the range\n-\t\t\t * \"v1.0..v2.0\")?\n-\t\t\t */\n-\t\t\tstruct commit *one = lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t\t &oid);\n+\t\tif (!oideq(&oid, &e->item->oid)) {\n+\t\t\tstruct commit *one=lookup_commit_reference(the_repository,\n+\t\t\t\t\t\t\t\t &oid);\n \t\t\tstruct object *obj;\n-\n \t\t\tif (e->item == &(one->object)) {\n-\t\t\t\t/*\n-\t\t\t\t * Need to include e->name as an\n-\t\t\t\t * independent ref to the pack-objects\n-\t\t\t\t * input, so that the tag is included\n-\t\t\t\t * in the output; otherwise we would\n-\t\t\t\t * end up triggering \"empty bundle\"\n-\t\t\t\t * error.\n-\t\t\t\t */\n \t\t\t\tobj = parse_object_or_die(&oid, e->name);\n \t\t\t\tobj->flags |= SHOWN;\n \t\t\t\tadd_pending_object(revs, obj, e->name);\n\nSo I really do want some way of saying \"all of the block, no matter what\nit is\". Or of leaving it out as context.\n\n-Peff\n"},{"id":"356426","messageId":"20180824001643.GA14259@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180824000637.GA10847@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T00:16:44Z","receivedAt":"2018-08-24T00:16:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 08:06:37PM -0400, Jeff King wrote:\n\n> This almost works:\n> \n> @@\n> expression a, b;\n> statement s;\n> @@\n> - if (oidcmp(a, b)) s\n> + if (!oideq(a, b)) s\n>\n> [...]\n\n> So I really do want some way of saying \"all of the block, no matter what\n> it is\". Or of leaving it out as context.\n\nAha. The magic invocation is:\n\n @@\n expression a, b;\n statement s;\n @@\n - if (oidcmp(a, b))\n + if (!oideq(a, b))\n s\n\nI would have expected that you could replace \"s\" with \"...\", but that\ndoes not seem to work.\n\n-Peff\n"},{"id":"356430","messageId":"CA+P7+xpm-gsjCpPOZ=2z03Peb1Jb6axKo2nTp=UUpAFgWNureg@mail.gmail.com","threadId":"49173","inReplyTo":"20180824001643.GA14259@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-08-24T02:48:42Z","receivedAt":"2018-08-24T02:48:57Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 23, 2018 at 5:16 PM Jeff King wrote:\n>\n> On Thu, Aug 23, 2018 at 08:06:37PM -0400, Jeff King wrote:\n>\n> > This almost works:\n> >\n> > @@\n> > expression a, b;\n> > statement s;\n> > @@\n> > - if (oidcmp(a, b)) s\n> > + if (!oideq(a, b)) s\n> >\n> > [...]\n>\n> > So I really do want some way of saying \"all of the block, no matter what\n> > it is\". Or of leaving it out as context.\n>\n> Aha. The magic invocation is:\n>\n> @@\n> expression a, b;\n> statement s;\n> @@\n> - if (oidcmp(a, b))\n> + if (!oideq(a, b))\n> s\n>\n> I would have expected that you could replace \"s\" with \"...\", but that\n> does not seem to work.\n>\n> -Peff\n\nOdd...\n\nWhat about..\n\n- if (oidcmp(a,b))\n+ if(!oideq(a,b))\n { ... }\n\nOr maybe you need to use something like\n\n <...\n- if (oidcmp(a,b))\n+ if (!oideq(a,b))\n ...>\n\nHmm. Yea, semantic patches are a bit confusing overall sometimes.\n\nBut it looks like you got something which works?\n\nThanks,\nJake\n"},{"id":"356433","messageId":"20180824025955.GA24535@sigill.intra.peff.net","threadId":"49173","inReplyTo":"CA+P7+xpm-gsjCpPOZ=2z03Peb1Jb6axKo2nTp=UUpAFgWNureg@mail.gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T02:59:55Z","receivedAt":"2018-08-24T03:01:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 07:48:42PM -0700, Jacob Keller wrote:\n\n> Odd...\n> \n> What about..\n> \n> - if (oidcmp(a,b))\n> + if(!oideq(a,b))\n> { ... }\n\nNope, it doesn't like that syntactically.\n\n> Or maybe you need to use something like\n> \n> <...\n> - if (oidcmp(a,b))\n> + if (!oideq(a,b))\n> ...>\n\nNor that (I also tried finding documentation on what exactly the angle\nbrackets mean, but couldn't).\n\n> Hmm. Yea, semantic patches are a bit confusing overall sometimes.\n> \n> But it looks like you got something which works?\n\nActually, what I showed earlier does seem to have some weirdness with\nelse-if. But I finally stumbled on something even better:\n\n - oidcmp(a, b) != 0\n + !oideq(a, b)\n\nBecause of the isomorphisms that coccinelle knows about, that catches\neverything we want. Obvious ones like:\n\ndiff --git a/bisect.c b/bisect.c\nindex 41c56a665e..7c1d8f1a6d 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -595,7 +595,7 @@ static struct commit_list *skip_away(struct commit_list *list, int count)\n \n for (i = 0; cur; cur = cur->next, i++) {\n if (i == index) {\n- if (oidcmp(&cur->item->object.oid, current_bad_oid))\n+ if (!oideq(&cur->item->object.oid, current_bad_oid))\n return cur;\n if (previous)\n return previous;\n\nand compound conditionals like:\n\ndiff --git a/blame.c b/blame.c\nindex 10d72e36dd..538d0ab1aa 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1834,7 +1834,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,\n\n sb->revs->children.name = \"children\";\n while (c->parents &&\n- oidcmp(&c->object.oid, &sb->final->object.oid)) {\n+ !oideq(&c->object.oid, &sb->final->object.oid)) {\n struct commit_list *l = xcalloc(1, sizeof(*l));\n\n l->item = c;\n\nand even non-if contexts, like:\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 631f6b9dc2..d85f4e93e1 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -825,7 +825,7 @@ int check_object_signature(const struct object_id *oid, void *map,\n \n if (map) {\n hash_object_file(map, size, type, &real_oid);\n- return oidcmp(oid, &real_oid) ? -1 : 0;\n+ return !oideq(oid, &real_oid) ? -1 : 0;\n }\n\nSo I think we have a winner. I'll polish that up into patches and send\nit out later tonight.\n\n-Peff\n"},{"id":"356438","messageId":"20180824064512.GA10521@sigill.intra.peff.net","threadId":"49173","inReplyTo":"20180824025955.GA24535@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T06:45:13Z","receivedAt":"2018-08-24T06:45:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 10:59:55PM -0400, Jeff King wrote:\n\n> So I think we have a winner. I'll polish that up into patches and send\n> it out later tonight.\n\nOof. This rabbit hole keeps going deeper and deeper. I wrote up my\ncoccinelle findings separately in:\n\n https://public-inbox.org/git/20180824064228.GA3183@sigill.intra.peff.net/\n\nwhich is possibly a coccinelle bug (there I talked about oidcmp, since\nit can be demonstrated with the existing transformations, but the same\nthing happens with my hasheq patches). I'll wait to see how that\ndiscussion plays out, but I do otherwise have hasheq() patches ready to\ngo, so it's probably not worth anybody else digging in further.\n\n-Peff\n"},{"id":"356439","messageId":"20180824065625.GA10556@sigill.intra.peff.net","threadId":"49173","inReplyTo":"4b0168ee-4826-1f14-fc83-04c4cec18687@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T06:56:25Z","receivedAt":"2018-08-24T06:56:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 04:59:27PM -0400, Derrick Stolee wrote:\n\n> Using git/git:\n> \n> Test      v2.18.0           v2.19.0-rc0             HEAD\n> -------------------------------------------------------------------------\n> 0001.2:   3.10(3.02+0.08)   3.27(3.17+0.09) +5.5% 3.14(3.02+0.11) +1.3%\n> \n> \n> Using torvalds/linux:\n> \n> Test     v2.18.0             v2.19.0-rc0               HEAD\n> ------------------------------------------------------------------------------\n> 0001.2:  56.08(45.91+1.50)   56.60(46.62+1.50) +0.9% 54.61(45.47+1.46) -2.6%\n\nInteresting that these timings aren't as dramatic as the ones you got\nthe other day (mine seemed to shift, too; for whatever reason it seems\nlike under load the difference is larger).\n\n> Now here is where I get on my soapbox (and create a TODO for myself later).\n> I ran the above with GIT_PERF_REPEAT_COUNT=10, which intuitively suggests\n> that the results should be _more_ accurate than the default of 3. However, I\n> then remember that we only report the *minimum* time from all the runs,\n> which is likely to select an outlier from the distribution. To test this, I\n> ran a few tests manually and found the variation between runs to be larger\n> than 3%.\n\nYes, I agree it's not a great system. The whole \"best of 3\" thing is\nOK for throwing out cold-cache warmups, but it's really bad for teasing\nout the significance of small changes, or even understanding how much\nrun-to-run noise there is.\n\n> When I choose my own metrics for performance tests, I like to run at least\n> 10 runs, remove the largest AND smallest runs from the samples, and then\n> take the average. I did this manually for 'git rev-list --all --objects' on\n> git/git and got the following results:\n\nI agree that technique is better. I wonder if there's something even\nmore statistically rigorous we could do. E.g., to compute the variance\nand throw away outliers based on standard deviations. And also to report\nthe variance to give a sense of the significance of any changes.\n\nObviously more runs gives greater confidence in the results, but 10\nsounds like a lot. Many of these tests take minutes to run. Letting it\ngo overnight is OK if you're doing a once-per-release mega-run, but it's\npretty painful if you just want to generate some numbers to show off\nyour commit.\n\n> v2.18.0    v2.19.0-rc0   HEAD\n> --------------------------------\n> 3.126 s    3.308 s       3.170 s\n\nSo that's 5% worsening in 2.19, and we reclaim all but 1.4% of it. Those\nnumbers match what I expect (and what I was seeing in some of my earlier\ntimings).\n\n> I just kicked off a script that will run this test on the Linux repo while I\n> drive home. I'll be able to report a similar table of data easily.\n\nThanks, I'd expect it to come up with similar percentages. So we'll see\nif that holds true. :)\n\n> My TODO is to consider aggregating the data this way (or with a median)\n> instead of reporting the minimum.\n\nYes, I think that would be a great improvement for t/perf.\n\n-Peff\n"},{"id":"356441","messageId":"87zhxcdwnb.fsf@evledraar.gmail.com","threadId":"49173","inReplyTo":"20180824065625.GA10556@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-24T07:57:44Z","receivedAt":"2018-08-24T07:57:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 24 2018, Jeff King wrote:\n\n> On Thu, Aug 23, 2018 at 04:59:27PM -0400, Derrick Stolee wrote:\n>\n>> Using git/git:\n>>\n>> Test v2.18.0 v2.19.0-rc0 HEAD\n>> -------------------------------------------------------------------------\n>> 0001.2: 3.10(3.02+0.08) 3.27(3.17+0.09) +5.5% 3.14(3.02+0.11) +1.3%\n>>\n>>\n>> Using torvalds/linux:\n>>\n>> Test v2.18.0 v2.19.0-rc0 HEAD\n>> ------------------------------------------------------------------------------\n>> 0001.2: 56.08(45.91+1.50) 56.60(46.62+1.50) +0.9% 54.61(45.47+1.46) -2.6%\n>\n> Interesting that these timings aren't as dramatic as the ones you got\n> the other day (mine seemed to shift, too; for whatever reason it seems\n> like under load the difference is larger).\n>\n>> Now here is where I get on my soapbox (and create a TODO for myself later).\n>> I ran the above with GIT_PERF_REPEAT_COUNT=10, which intuitively suggests\n>> that the results should be _more_ accurate than the default of 3. However, I\n>> then remember that we only report the *minimum* time from all the runs,\n>> which is likely to select an outlier from the distribution. To test this, I\n>> ran a few tests manually and found the variation between runs to be larger\n>> than 3%.\n>\n> Yes, I agree it's not a great system. The whole \"best of 3\" thing is\n> OK for throwing out cold-cache warmups, but it's really bad for teasing\n> out the significance of small changes, or even understanding how much\n> run-to-run noise there is.\n>\n>> When I choose my own metrics for performance tests, I like to run at least\n>> 10 runs, remove the largest AND smallest runs from the samples, and then\n>> take the average. I did this manually for 'git rev-list --all --objects' on\n>> git/git and got the following results:\n>\n> I agree that technique is better. I wonder if there's something even\n> more statistically rigorous we could do. E.g., to compute the variance\n> and throw away outliers based on standard deviations. And also to report\n> the variance to give a sense of the significance of any changes.\n>\n> Obviously more runs gives greater confidence in the results, but 10\n> sounds like a lot. Many of these tests take minutes to run. Letting it\n> go overnight is OK if you're doing a once-per-release mega-run, but it's\n> pretty painful if you just want to generate some numbers to show off\n> your commit.\n\nAn ex-coworker who's a *lot* smarter about these things than I am wrote\nthis module: https://metacpan.org/pod/Dumbbench\n\nI while ago I dabbled briefly with integrating it into t/perf/ but got\ndistracted by something else:\n\n The module currently works similar to [more traditional iterative\n benchmark modules], except (in layman terms) it will run the command\n many times, estimate the uncertainty of the result and keep\n iterating until a certain user-defined precision has been\n reached. Then, it calculates the resulting uncertainty and goes\n through some pain to discard bad runs and subtract overhead from the\n timings. The reported timing includes an uncertainty, so that\n multiple benchmarks can more easily be compared.\n\nDetails of how it works here:\nhttps://metacpan.org/pod/Dumbbench#HOW-IT-WORKS-AND-WHY-IT-DOESN'T\n\nSomething like that seems to me to be an inherently better\napproach. I.e. we have lots of test cases that take 500ms, and some that\ntake maybe 5 minutes (depending on the size of the repository).\n\nIndiscriminately repeating all of those for GIT_PERF_REPEAT_COUNT must\nbe dumber than something like the above method.\n\nWe could also speed up the runtime of the perf tests a lot with such a\nmethod, by e.g. saying that we're OK with less certainty on tests that\ntake a longer time than those that take a shorter time.\n\n\n>> v2.18.0 v2.19.0-rc0 HEAD\n>> --------------------------------\n>> 3.126 s 3.308 s 3.170 s\n>\n> So that's 5% worsening in 2.19, and we reclaim all but 1.4% of it. Those\n> numbers match what I expect (and what I was seeing in some of my earlier\n> timings).\n>\n>> I just kicked off a script that will run this test on the Linux repo while I\n>> drive home. I'll be able to report a similar table of data easily.\n>\n> Thanks, I'd expect it to come up with similar percentages. So we'll see\n> if that holds true. :)\n>\n>> My TODO is to consider aggregating the data this way (or with a median)\n>> instead of reporting the minimum.\n>\n> Yes, I think that would be a great improvement for t/perf.\n>\n> -Peff\n"},{"id":"356443","messageId":"3c73a374-ddc7-2c6b-5578-1b695c0ea7e0@gmail.com","threadId":"49173","inReplyTo":"20180824064512.GA10521@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-24T11:04:51Z","receivedAt":"2018-08-24T11:04:57Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2018 2:45 AM, Jeff King wrote:\n> On Thu, Aug 23, 2018 at 10:59:55PM -0400, Jeff King wrote:\n>\n>> So I think we have a winner. I'll polish that up into patches and send\n>> it out later tonight.\n> Oof. This rabbit hole keeps going deeper and deeper. I wrote up my\n> coccinelle findings separately in:\n>\n> https://public-inbox.org/git/20180824064228.GA3183@sigill.intra.peff.net/\n>\n> which is possibly a coccinelle bug (there I talked about oidcmp, since\n> it can be demonstrated with the existing transformations, but the same\n> thing happens with my hasheq patches). I'll wait to see how that\n> discussion plays out, but I do otherwise have hasheq() patches ready to\n> go, so it's probably not worth anybody else digging in further.\n>\nI was trying this myself yesterday, and found a doc _somewhere_ that \nsaid what we should do is this:\n\n@@\nexpression e1, e2;\n@@\nif (\n- oidcmp(e1, e2)\n+ !oideq(e1, e2)\n  )\n\n(Don't forget the space before the last end-paren!)\n\n-Stolee\n"},{"id":"356479","messageId":"719d5e65-553c-47a1-0b79-41c9af356f3a@gmail.com","threadId":"49173","inReplyTo":"4b0168ee-4826-1f14-fc83-04c4cec18687@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-24T16:45:10Z","receivedAt":"2018-08-24T16:45:15Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2018 4:59 PM, Derrick Stolee wrote:\n>\n> When I choose my own metrics for performance tests, I like to run at \n> least 10 runs, remove the largest AND smallest runs from the samples, \n> and then take the average. I did this manually for 'git rev-list --all \n> --objects' on git/git and got the following results:\n>\n> v2.18.0    v2.19.0-rc0   HEAD\n> --------------------------------\n> 3.126 s    3.308 s       3.170 s\n>\n> For full disclosure, here is a full table including all samples:\n>\n> |      | v2.18.0 | v2.19.0-rc0 | HEAD    |\n> |------|---------|-------------|---------|\n> |      | 4.58    | 3.302       | 3.239   |\n> |      | 3.13    | 3.337       | 3.133   |\n> |      | 3.213   | 3.291       | 3.159   |\n> |      | 3.219   | 3.318       | 3.131   |\n> |      | 3.077   | 3.302       | 3.163   |\n> |      | 3.074   | 3.328       | 3.119   |\n> |      | 3.022   | 3.277       | 3.125   |\n> |      | 3.083   | 3.259       | 3.203   |\n> |      | 3.057   | 3.311       | 3.223   |\n> |      | 3.155   | 3.413       | 3.225   |\n> | Max  | 4.58    | 3.413       | 3.239   |\n> | Min  | 3.022   | 3.259       | 3.119   |\n> | Avg* | 3.126   | 3.30825     | 3.17025 |\n>\n> (Note that the largest one was the first run, on v2.18.0, which is due \n> to a cold disk.)\n>\n> I just kicked off a script that will run this test on the Linux repo \n> while I drive home. I'll be able to report a similar table of data \n> easily.\n\nHere are the numbers for Linux:\n\n|      | v2.18.0  | v2.19.0-rc0 | HEAD   |\n|------|----------|-------------|--------|\n|      | 86.5     | 70.739      | 57.266 |\n|      | 60.582   | 101.928     | 56.641 |\n|      | 58.964   | 60.139      | 60.258 |\n|      | 59.47    | 61.141      | 58.213 |\n|      | 62.554   | 60.73       | 84.54  |\n|      | 59.139   | 85.424      | 57.745 |\n|      | 58.487   | 59.31       | 59.979 |\n|      | 58.653   | 69.845      | 60.181 |\n|      | 58.085   | 102.777     | 61.455 |\n|      | 58.304   | 60.459      | 62.551 |\n| Max  | 86.5     | 102.777     | 84.54  |\n| Min  | 58.085   | 59.31       | 56.641 |\n| Avg* | 59.51913 | 71.30063    | 59.706 |\n| Med  | 59.0515  | 65.493      | 60.08  |\n\nThanks,\n\n-Stolee\n\n"},{"id":"356514","messageId":"20180825082641.GC1672@sigill.intra.peff.net","threadId":"49173","inReplyTo":"719d5e65-553c-47a1-0b79-41c9af356f3a@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-25T08:26:42Z","receivedAt":"2018-08-25T08:26:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 24, 2018 at 12:45:10PM -0400, Derrick Stolee wrote:\n\n> Here are the numbers for Linux:\n> \n> |      | v2.18.0  | v2.19.0-rc0 | HEAD   |\n> |------|----------|-------------|--------|\n> |      | 86.5     | 70.739      | 57.266 |\n> |      | 60.582   | 101.928     | 56.641 |\n> |      | 58.964   | 60.139      | 60.258 |\n> |      | 59.47    | 61.141      | 58.213 |\n> |      | 62.554   | 60.73       | 84.54  |\n> |      | 59.139   | 85.424      | 57.745 |\n> |      | 58.487   | 59.31       | 59.979 |\n> |      | 58.653   | 69.845      | 60.181 |\n> |      | 58.085   | 102.777     | 61.455 |\n> |      | 58.304   | 60.459      | 62.551 |\n> | Max  | 86.5     | 102.777     | 84.54  |\n> | Min  | 58.085   | 59.31       | 56.641 |\n> | Avg* | 59.51913 | 71.30063    | 59.706 |\n> | Med  | 59.0515  | 65.493      | 60.08  |\n\nThanks. The median ones are the most interesting, I think (and show a\nvery satisfying recovery from my patch).\n\nI'm surprised at the variance, especially in your v2.19 runs. It makes\nme distrust the mean (and implies to me we could do better by throwing\naway outliers based on value, not just the single high/low; or just\nusing the median also solves that).\n\nThe variance in the v2.18 column is what I'd expect based on past\nexperience (slow cold cache to start, then a few percent change\nrun-to-run).\n\n-Peff\n"},{"id":"356603","messageId":"xmqqbm9nk3f6.fsf@gitster-ct.c.googlers.com","threadId":"49173","inReplyTo":"20180824025955.GA24535@sigill.intra.peff.net","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-27T19:36:13Z","receivedAt":"2018-08-27T19:36:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, what I showed earlier does seem to have some weirdness with\n> else-if. But I finally stumbled on something even better:\n>\n> - oidcmp(a, b) != 0\n> + !oideq(a, b)\n>\n> Because of the isomorphisms that coccinelle knows about, that catches\n> everything we want.\n\nNice ;-)\n\n"},{"id":"357191","messageId":"39ec35843c3164a9da75466b768636cb7871690d.camel@gmail.com","threadId":"49173","inReplyTo":"f854aba0-6d28-7f2b-aad2-858983c4af36@gmail.com","subject":"Re: [ANNOUNCE] Git v2.19.0-rc0","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-09-02T18:53:41Z","receivedAt":"2018-09-02T18:54:03Z","isPatch":false,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Thu, 2018-08-23 at 06:26 -0400, Derrick Stolee wrote:\n> \n> Around the time that my proposed approaches were getting vetoed for \n> alignment issues, I figured I was out of my depth here. I reached out to \n> Daniel Lemire (of EWAH bitmap fame) on Twitter [1]. His blog is full of \n> posts of word-based approaches to different problems, so I thought he \n> might know something off the top of his head that would be applicable. \n> His conclusion (after looking only a short time) was to take a 'hasheq' \n> approach [2] like Peff suggested [3]. Since that requires auditing all \n> callers of hashcmp to see if hasheq is appropriate, it is not a good \n> solution for 2.19 but (in my opinion) should be evaluated as part of the \n> 2.20 cycle.\n> \n\nThat was an interesting blog post, indeed. It had an interesting\ncomments section. One comment especially caught my eyes was [a]:\n\n\"So the way gcc (and maybe clang) handles this is specifically by\nrecognizing memcmp and checking whether a only a 2-way result is needed\nand then essentially replacing it with a memcmp_eq call.\n\n...\"\n\nI find this to be an interesting note. It seems GCC does optimize when\nwe clearly indicate that we use the result of the memcmp as a boolean.\nSo would that help in anyway? Maybe it would help in writing a `hasheq`\nmethod easily? I'm not sure.\n\n> [1] https://twitter.com/stolee/status/1032312965754748930\n> \n> [2] \n> https://lemire.me/blog/2018/08/22/avoid-lexicographical-comparisons-when-testing-for-string-equality/\n> \n> [3] \n> https://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/\n> \n> [4] \n> https://public-inbox.org/git/7ea416cf-b043-1274-e161-85a8780b8e1c@gmail.com/\n\n\n[a]: \nhttps://lemire.me/blog/2018/08/22/avoid-lexicographical-comparisons-when-testing-for-string-equality/#comment-344073\n\n--\nSivaraam\n\n"}]}