* What's cooking in git.git (Apr 2019, #05; Thu, 25) @ 2019-04-25 10:15 Junio C Hamano 2019-04-25 14:50 ` Elijah Newren ` (4 more replies) 0 siblings, 5 replies; 38+ messages in thread From: Junio C Hamano @ 2019-04-25 10:15 UTC (permalink / raw) To: git Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -------------------------------------------------- [Graduated to "master"] * ab/commit-graph-fixes (2019-04-01) 8 commits (merged to 'next' on 2019-04-16 at 97f4ba02f0) + commit-graph: improve & i18n error messages + commit-graph write: don't die if the existing graph is corrupt + commit-graph verify: detect inability to read the graph + commit-graph: don't pass filename to load_commit_graph_one_fd_st() + commit-graph: don't early exit(1) on e.g. "git status" + commit-graph: fix segfault on e.g. "git status" + commit-graph tests: test a graph that's too small + commit-graph tests: split up corrupt_graph_and_verify() (this branch is used by ds/commit-graph-format-v2.) Code cleanup with more careful error checking before using data read from the commit-graph file. * ab/gc-docs (2019-04-08) 11 commits (merged to 'next' on 2019-04-22 at 02785d40f5) + gc docs: remove incorrect reference to gc.auto=0 + gc docs: clarify that "gc" doesn't throw away referenced objects + gc docs: note "gc --aggressive" in "fast-import" + gc docs: downplay the usefulness of --aggressive + gc docs: note how --aggressive impacts --window & --depth + gc docs: fix formatting for "gc.writeCommitGraph" + gc docs: re-flow the "gc.*" section in "config" + gc docs: include the "gc.*" section from "config" in "gc" + gc docs: clean grammar for "gc.bigPackThreshold" + gc docs: stop noting "repack" flags + gc docs: modernize the advice for manually running "gc" Update docs around "gc". * ab/gc-reflog (2019-04-01) 7 commits (merged to 'next' on 2019-04-16 at aa27f951a8) + gc: handle & check gc.reflogExpire config + reflog tests: assert lack of early exit with expiry="never" + reflog tests: test for the "points nowhere" warning + reflog tests: make use of "test_config" idiom + gc: refactor a "call me once" pattern + gc: convert to using the_hash_algo + gc: remove redundant check for gc_auto_threshold Fix various glitches in "git gc" around reflog handling. * ab/test-lib-pass-trace2-env (2019-04-01) 1 commit (merged to 'next' on 2019-04-16 at 4dad6d6d7a) + test-lib: whitelist GIT_TR2_* in the environment Allow tracing of Git executable while running the testsuite. * ag/sequencer-reduce-rewriting-todo (2019-03-07) 18 commits (merged to 'next' on 2019-04-10 at 7eab7c7800) + rebase--interactive: move transform_todo_file() + sequencer: use edit_todo_list() in complete_action() + rebase-interactive: rewrite edit_todo_list() to handle the initial edit + rebase-interactive: append_todo_help() changes + rebase-interactive: use todo_list_write_to_file() in edit_todo_list() + sequencer: refactor skip_unnecessary_picks() to work on a todo_list + rebase--interactive: move rearrange_squash_in_todo_file() + rebase--interactive: move sequencer_add_exec_commands() + sequencer: change complete_action() to use the refactored functions + sequencer: make sequencer_make_script() write its script to a strbuf + sequencer: refactor rearrange_squash() to work on a todo_list + sequencer: refactor sequencer_add_exec_commands() to work on a todo_list + sequencer: refactor check_todo_list() to work on a todo_list + sequencer: introduce todo_list_write_to_file() + sequencer: refactor transform_todos() to work on a todo_list + sequencer: remove the 'arg' field from todo_item + sequencer: make the todo_list structure public + sequencer: changes in parse_insn_buffer() (this branch is used by pw/rebase-i-internal.) The scripted version of "git rebase -i" wrote and rewrote the todo list many times during a single step of its operation, and the recent C-rewrite made a faithful conversion of the logic to C. The implementation has been updated to carry necessary information around in-core to avoid rewriting the same file over and over unnecessarily. cf. <20190305191805.13561-1-alban.gruin@gmail.com> (v8) * bc/hash-transition-16 (2019-04-01) 35 commits (merged to 'next' on 2019-04-16 at 8227fea6fa) + gitweb: make hash size independent + Git.pm: make hash size independent + read-cache: read data in a hash-independent way + dir: make untracked cache extension hash size independent + builtin/difftool: use parse_oid_hex + refspec: make hash size independent + archive: convert struct archiver_args to object_id + builtin/get-tar-commit-id: make hash size independent + get-tar-commit-id: parse comment record + hash: add a function to lookup hash algorithm by length + remote-curl: make hash size independent + http: replace sha1_to_hex + http: compute hash of downloaded objects using the_hash_algo + http: replace hard-coded constant with the_hash_algo + http-walker: replace sha1_to_hex + http-push: remove remaining uses of sha1_to_hex + http-backend: allow 64-character hex names + http-push: convert to use the_hash_algo + builtin/pull: make hash-size independent + builtin/am: make hash size independent + fast-import: replace sha1_to_hex + fast-import: make hash-size independent + builtin/name-rev: make hash-size independent + object-store: rename and expand packed_git's sha1 member + notes: replace sha1_to_hex + notes: make hash size independent + notes-merge: switch to use the_hash_algo + submodule: avoid hard-coded constants + pack-bitmap: switch hash tables to use struct object_id + pack-bitmap: switch hard-coded constants to the_hash_algo + pack-bitmap: replace sha1_to_hex + pack-bitmap: convert struct stored_bitmap to object_id + pack-bitmap: make bitmap header handling hash agnostic + khash: move oid hash table definition + t/lib-submodule-update: use appropriate length constant Conversion from unsigned char[20] to struct object_id continues. * bc/send-email-qp-cr (2019-04-14) 1 commit (merged to 'next' on 2019-04-22 at 69398b0ea8) + send-email: default to quoted-printable when CR is present "git send-email" has been taught to use quoted-printable when the payload contains carriage-return. The use of the mechanism is in line with the design originally added the codepath that chooses QP when the payload has overly long lines. * bp/post-index-change-hook (2019-02-15) 1 commit (merged to 'next' on 2019-03-11 at cb96d1d7c4) + read-cache: add post-index-change hook Originally merged to 'next' on 2019-02-23 A new hook "post-index-change" is called when the on-disk index file changes, which can help e.g. a virtualized working tree implementation. * bs/sendemail-tighten-anything-by (2019-04-04) 1 commit (merged to 'next' on 2019-04-22 at 0a0680f234) + send-email: don't cc *-by lines with '-' prefix The recently added feature to add addresses that are on anything-by: trailers in 'git send-email' was found to be way too eager and considered nonsense strings as if they can be legitimate beginning of *-by: trailer. This has been tightened. * dk/blame-keep-origin-blob (2019-04-03) 1 commit (merged to 'next' on 2019-04-16 at 39679dde8b) + blame.c: don't drop origin blobs as eagerly Performance fix around "git blame", especially in a linear history (which is the norm we should optimize for). * dl/submodule-set-branch (2019-04-10) 3 commits (merged to 'next' on 2019-04-22 at 3b640715ae) + submodule: teach set-branch subcommand + submodule--helper: teach config subcommand --unset + git-submodule.txt: "--branch <branch>" option defaults to 'master' "git submodule" learns "set-branch" subcommand that allows the submodule.*.branch settings to be modified. * en/fast-import-parsing-fix (2019-04-01) 5 commits (merged to 'next' on 2019-04-16 at b318831bde) + fast-import: fix erroneous handling of get-mark with empty orphan commits + fast-import: only allow cat-blob requests where it makes sense + fast-import: check most prominent commands first + git-fast-import.txt: fix wording about where ls command can appear + t9300: demonstrate bug with get-mark and empty orphan commits "git fast-import" update. * jc/gettext-test-fix (2019-04-15) 1 commit (merged to 'next' on 2019-04-22 at 7c57deeb04) + gettext tests: export the restored GIT_TEST_GETTEXT_POISON The GETTEXT_POISON test option has been quite broken ever since it was made runtime-tunable, which has been fixed. * jk/fetch-reachability-error-fix (2019-04-15) 7 commits (merged to 'next' on 2019-04-22 at b4ce8375c0) + fetch: do not consider peeled tags as advertised tips + remote.c: make singular free_ref() public + fetch: use free_refs() + pkt-line: prepare buffer before handling ERR packets + upload-pack: send ERR packet for non-tip objects + t5530: check protocol response for "not our ref" + t5516: drop ok=sigpipe from unreachable-want tests Code clean-up and a fix for "git fetch" by an explicit object name (as opposed to fetching refs by name). * jk/revision-rewritten-parents-in-prio-queue (2019-04-04) 1 commit (merged to 'next' on 2019-04-16 at bdc1465128) + revision: use a prio_queue to hold rewritten parents Performance fix for "rev-list --parents -- pathspec". * jk/server-info-rabbit-hole (2019-04-16) 13 commits (merged to 'next' on 2019-04-16 at 3dded8be9a) + update_info_refs(): drop unused force parameter + server-info: drop objdirlen pointer arithmetic + server-info: drop nr_alloc struct member + server-info: use strbuf to read old info/packs file + server-info: simplify cleanup in parse_pack_def() + server-info: fix blind pointer arithmetic + http: simplify parsing of remote objects/info/packs + packfile: fix pack basename computation + midx: check both pack and index names for containment + t5319: drop useless --buffer from cat-file + t5319: fix bogus cat-file argument + pack-revindex: open index if necessary + packfile.h: drop extern from function declarations Code clean-up around a much-less-important-than-it-used-to-be update_server_info() funtion. * jk/unused-params-even-more (2019-03-21) 13 commits (merged to 'next' on 2019-04-10 at 12edf8872f) + parse_opt_ref_sorting: always use with NONEG flag + pretty: drop unused strbuf from parse_padding_placeholder() + pretty: drop unused "type" parameter in needs_rfc2047_encoding() + parse-options: drop unused ctx parameter from show_gitcomp() + fetch_pack(): drop unused parameters + report_path_error(): drop unused prefix parameter + unpack-trees: drop unused error_type parameters + unpack-trees: drop name_entry from traverse_by_cache_tree() + test-date: drop unused "now" parameter from parse_dates() + update-index: drop unused prefix_length parameter from do_reupdate() + log: drop unused "len" from show_tagger() + log: drop unused rev_info from early output + revision: drop some unused "revs" parameters Code cleanup. * jk/xmalloc (2019-04-12) 4 commits (merged to 'next' on 2019-04-22 at 1a907289fa) + progress: use xmalloc/xcalloc + xdiff: use xmalloc/xrealloc + xdiff: use git-compat-util + test-prio-queue: use xmalloc The code is updated to check the result of memory allocation before it is used in more places, by using xmalloc and/or xcalloc calls. * js/difftool-no-index (2019-03-18) 3 commits (merged to 'next' on 2019-04-16 at 7313f9ff18) + difftool: allow running outside Git worktrees with --no-index + parse-options: make OPT_ARGUMENT() more useful + difftool: remove obsolete (and misleading) comment "git difftool" can now run outside a repository. * js/iso8895-test-on-apfs (2019-04-15) 1 commit (merged to 'next' on 2019-04-22 at c2fadead33) + t9822: skip tests if file names cannot be ISO-8859-1 encoded Test fix on APFS that is incapable of store paths in Latin-1. * js/macos-gettext-build (2019-04-15) 1 commit (merged to 'next' on 2019-04-22 at de4cbb1431) + macOS: make sure that gettext is found Build with gettext breaks on recent macOS w/ Homebrew when /usr/local/bin is not on PATH, which has been corrected. * js/t3301-unbreak-notes-test (2019-04-09) 1 commit (merged to 'next' on 2019-04-22 at a015b00bd9) + t3301: fix false negative Test fix. * js/untracked-cache-allocfix (2019-04-12) 1 commit (merged to 'next' on 2019-04-22 at 004a544075) + untracked cache: fix off-by-one (this branch is used by jk/untracked-cache-more-fixes.) An underallocation in the code to read the untracked cache extension has been corrected. * jt/batch-fetch-blobs-in-diff (2019-04-08) 2 commits (merged to 'next' on 2019-04-22 at 0598bae567) + diff: batch fetching of missing blobs + sha1-file: support OBJECT_INFO_FOR_PREFETCH (this branch is used by cc/multi-promisor.) While running "git diff" in a lazy clone, we can upfront know which missing blobs we will need, instead of waiting for the on-demand machinery to discover them one by one. Aim to achieve better performance by batching the request for these promised blobs. * jt/fetch-no-update-shallow-in-proto-v2 (2019-04-01) 3 commits (merged to 'next' on 2019-04-16 at 05c5ebe471) + fetch-pack: respect --no-update-shallow in v2 + fetch-pack: call prepare_shallow_info only if v0 + Merge branch 'jt/test-protocol-version' into jt/fetch-no-update-shallow-in-proto-v2 Fix for protocol v2 support in "git fetch-pack" of shallow clones. * jt/fetch-pack-wanted-refs-optim (2019-04-01) 1 commit (merged to 'next' on 2019-04-16 at 051f6bd38a) + fetch-pack: binary search when storing wanted-refs Performance fix around "git fetch" that grabs many refs. * km/t3000-retitle (2019-04-12) 1 commit (merged to 'next' on 2019-04-22 at 2d5aa01ca6) + t3000 (ls-files -o): widen description to reflect current tests A test update. * nd/checkout-m (2019-03-24) 4 commits (merged to 'next' on 2019-04-16 at 4d7c322bed) + checkout: prevent losing staged changes with --merge + read-tree: add --quiet + unpack-trees: rename "gently" flag to "quiet" + unpack-trees: keep gently check inside add_rejected_path "git checkout -m <other>" was about carrying the differences between HEAD and the working-tree files forward while checking out another branch, and ignored the differences between HEAD and the index. The command has been taught to abort when the index and the HEAD are different. * nd/commit-a-with-paths-msg-update (2019-03-22) 1 commit (merged to 'next' on 2019-04-16 at a36c712b39) + commit: improve error message in "-a <paths>" case The message given when "git commit -a <paths>" errors out has been updated. * nd/diff-parseopt-4 (2019-03-24) 20 commits (merged to 'next' on 2019-04-10 at 893b135f10) + am: avoid diff_opt_parse() + diff --no-index: use parse_options() instead of diff_opt_parse() + range-diff: use parse_options() instead of diff_opt_parse() + diff.c: allow --no-color-moved-ws + diff-parseopt: convert --color-moved-ws + diff-parseopt: convert --[no-]color-moved + diff-parseopt: convert --inter-hunk-context + diff-parseopt: convert --no-prefix + diff-parseopt: convert --line-prefix + diff-parseopt: convert --[src|dst]-prefix + diff-parseopt: convert --[no-]abbrev + diff-parseopt: convert --diff-filter + diff-parseopt: convert --find-object + diff-parseopt: convert -O + diff-parseopt: convert --pickaxe-all|--pickaxe-regex + diff-parseopt: convert -S|-G + diff-parseopt: convert -l + diff-parseopt: convert -z + diff-parseopt: convert --ita-[in]visible-in-index + diff-parseopt: convert --ws-error-highlight Fourth batch to teach the diff machinery to use the parse-options API. * nd/submodule-foreach-quiet (2019-04-15) 1 commit (merged to 'next' on 2019-04-22 at bf982bca7b) + submodule foreach: fix "<command> --quiet" not being respected "git submodule foreach <command> --quiet" did not pass the option down correctly, which has been corrected. * po/describe-not-necessarily-7 (2019-04-08) 1 commit (merged to 'next' on 2019-04-22 at 65b47ca73f) + describe doc: remove '7-char' abbreviation reference Docfix. * po/rerere-doc-fmt (2019-04-08) 1 commit (merged to 'next' on 2019-04-22 at 780c0d2450) + rerere doc: quote `rerere.enabled` Docfix. * pw/cherry-pick-continue (2019-03-18) 3 commits (merged to 'next' on 2019-04-16 at 1bfd7a7179) + cherry-pick --continue: remember options + cherry-pick: demonstrate option amnesia + sequencer: break some long lines "git cherry-pick --options A..B", after giving control back to the user to ask help resolving a conflicted step, did not honor the options it originally received, which has been corrected. * sg/blame-in-bare-start-at-head (2019-04-08) 1 commit (merged to 'next' on 2019-04-22 at 159777c280) + blame: default to HEAD in a bare repo when no start commit is given "git blame -- path" in a non-bare repository starts blaming from the working tree, and the same command in a bare repository errors out because there is no working tree by definition. The command has been taught to instead start blaming from the commit at HEAD, which is more useful. * sg/index-pack-progress (2019-04-01) 1 commit (merged to 'next' on 2019-04-16 at a10bfdd950) + index-pack: show progress while checking objects A progress indicator has been added to the "index-pack" step, which often makes users wait for completion during "git clone". * sg/overlong-progress-fix (2019-04-15) 4 commits (merged to 'next' on 2019-04-22 at 69921cdf09) + progress: break too long progress bar lines + progress: clear previous progress update dynamically + progress: assemble percentage and counters in a strbuf before printing + progress: make display_progress() return void Updating the display with progress message has been cleaned up to deal better with overlong messages. * sg/test-atexit (2019-03-14) 11 commits (merged to 'next' on 2019-04-10 at 7839135291) + t9811-git-p4-label-import: fix pipeline negation + git p4 test: disable '-x' tracing in the p4d watchdog loop + git p4 test: simplify timeout handling + git p4 test: clean up the p4d cleanup functions + git p4 test: use 'test_atexit' to kill p4d and the watchdog process + t0301-credential-cache: use 'test_atexit' to stop the credentials helper + tests: use 'test_atexit' to stop httpd + git-daemon: use 'test_atexit` to stop 'git-daemon' + test-lib: introduce 'test_atexit' + t/lib-git-daemon: make sure to kill the 'git-daemon' process + test-lib: fix interrupt handling with 'dash' and '--verbose-log -x' Test framework update to more robustly clean up leftover files and processes after tests are done. * tg/ls-files-debug-format-fix (2019-04-08) 1 commit (merged to 'next' on 2019-04-22 at a5ac1ca49f) + ls-files: use correct format string Debugging code fix. * tz/doc-apostrophe-no-longer-needed (2019-04-10) 1 commit (merged to 'next' on 2019-04-22 at 8ff03863ce) + Documentation/git-show-branch: avoid literal {apostrophe} Doc formatting fix. -------------------------------------------------- [New Topics] * jc/make-dedup-ls-files-output (2019-04-22) 1 commit - Makefile: dedup list of files obtained from ls-files A "ls-files" that emulates "find" to enumerate files in the working tree resulted in duplicated Makefile rules that caused the build to issue an unnecessary warning during a trial build after merge conflicts are resolved in working tree *.h files but before the resolved results are added to the index. This has been corrected. Will merge to 'next'. A not-so-low hanging fruit to teach ls-files to dedup either optionally or always has also been discussed, which probably is a good idea, to prevent mistakes similar to the bug this topic fixes in the future. * jk/ls-files-doc-markup-fix (2019-04-23) 1 commit - doc/ls-files: put nested list for "-t" option into block Docfix. Will merge to 'next'. * jk/p5302-avoid-collision-check-cost (2019-04-23) 1 commit - p5302: create the repo in each index-pack test Fix index-pack perf test so that the repeated invocations always run in an empty repository, which emulates the initial clone situation better. Will merge to 'next'. * dl/rev-tilde-doc-clarify (2019-04-24) 1 commit - revisions.txt: mention <rev>~ form Docfix. Will merge to 'next'. * en/unicode-in-refnames (2019-04-24) 1 commit - Honor core.precomposeUnicode in more places The names of the refs stored as filesystem entities may become different from what the end-user expects, just like files in the working tree gets "renamed", on a filesystem like HFS+. Work it around by paying attemption to the core.precomposeUnicode configuration. Looked sensible. Ready for next? * jk/perf-aggregate-wo-libjson (2019-04-24) 1 commit - t/perf: depend on perl JSON only when using --codespeed The script to aggregate perf result unconditionally depended on libjson-perl even though it did not have to, which has been corrected. Will merge to 'next'. * cc/access-on-aix-workaround (2019-04-25) 1 commit - git-compat-util: work around for access(X_OK) under root (this branch uses cc/aix-has-fileno-as-a-macro.) * dl/difftool-mergetool (2019-04-25) 6 commits - difftool: fallback on merge.guitool - difftool: make --gui, --tool and --extcmd mutually exclusive - mergetool: fallback to tool when guitool unavailable - mergetool: use get_merge_tool function - t7610: add mergetool --gui tests - t7610: unsuppress output -------------------------------------------------- [Stalled] * nb/branch-show-other-worktrees-head (2019-03-18) 3 commits - branch: add worktree info on verbose output - branch: update output to include worktree info - ref-filter: add worktreepath atom "git branch --list" learned to show branches that are checked out in other worktrees connected to the same repository prefixed with '+', similar to the way the currently checked out branch is shown with '*' in front. Getting there... cf. <20190316013807.38756-1-nbelakovski@gmail.com> (v9) cf. <20190318121054.GC24175@szeder.dev> * jt/fetch-cdn-offload (2019-03-12) 9 commits - SQUASH??? - upload-pack: send part of packfile response as uri - fetch-pack: support more than one pack lockfile - upload-pack: refactor reading of pack-objects out - Documentation: add Packfile URIs design doc - Documentation: order protocol v2 sections - http-fetch: support fetching packfiles by URL - http: improve documentation of http_pack_request - http: use --stdin when getting dumb HTTP pack WIP for allowing a response to "git fetch" to instruct the bulk of the pack contents to be instead taken from elsewhere (aka CDN). Still being discussed. * js/add-i-coalesce-after-editing-hunk (2018-08-28) 1 commit - add -p: coalesce hunks before testing applicability Applicability check after a patch is edited in a "git add -i/p" session has been improved. Will hold. cf. <e5b2900a-0558-d3bf-8ea1-d526b078bbc2@talktalk.net> * js/protocol-advertise-multi (2018-12-28) 1 commit - protocol: advertise multiple supported versions The transport layer has been updated so that the protocol version used can be negotiated between the parties, by the initiator listing the protocol versions it is willing to talk, and the other side choosing from one of them. Expecting a reroll. cf. <CANq=j3u-zdb_FvNJGPCmygNMScseav63GhVvBX3NcVS4f7TejA@mail.gmail.com> * mk/use-size-t-in-zlib (2018-10-15) 1 commit - zlib.c: use size_t for size The wrapper to call into zlib followed our long tradition to use "unsigned long" for sizes of regions in memory, which have been updated to use "size_t". * dl/remote-save-to-push (2018-12-11) 1 commit - remote: add --save-to-push option to git remote set-url "git remote set-url" learned a new option that moves existing value of the URL field to pushURL field of the remote before replacing the URL field with a new value. Anybody who wants to champion this topic? I am personally not yet quite convinced if this is worth pursuing. -------------------------------------------------- [Cooking] * ds/midx-expire-repack (2019-04-25) 11 commits - t5319-multi-pack-index.sh: test batch size zero - midx: add test that 'expire' respects .keep files - multi-pack-index: test expire while adding packs - midx: implement midx_repack() - multi-pack-index: prepare 'repack' subcommand - multi-pack-index: implement 'expire' subcommand - midx: refactor permutation logic and pack sorting - midx: simplify computation of pack name lengths - multi-pack-index: prepare for 'expire' subcommand - Docs: rearrange subcommands for multi-pack-index - repack: refactor pack deletion for future use "git multi-pack-index expire/repack" are new subcommands that consult midx file and are used to drop unused pack files and coalesce small pack files that are still in use. cf. <20190424151428.170316-1-dstolee@microsoft.com> (v5) * es/first-contrib-tutorial (2019-04-25) 1 commit - documentation: add tutorial for first contribution A new tutorial targetting specifically aspiring git-core developers. cf. <20190423193410.101803-1-emilyshaffer@google.com> (v4) * pw/clean-sequencer-state-upon-final-commit (2019-04-17) 2 commits - fix cherry-pick/revert status after commit - commit/reset: try to clean up sequencer state "git chery-pick" (and "revert" that shares the same runtime engine) that deals with multiple commits got confused when the final step gets stopped with a conflict and the user concluded the sequence with "git commit". Attempt to fix it by cleaning up the state files used by these commands in such a situation. Will merge to 'next'. * pw/sequencer-cleanup-with-signoff-x-fix (2019-04-18) 1 commit (merged to 'next' on 2019-04-25 at cc587fb2b9) + sequencer: fix cleanup with --signoff and -x (this branch is used by dl/merge-cleanup-scissors-fix.) "git cherry-pick" run with the "-x" or the "--signoff" option used to (and more importantly, ought to) clean up the commit log message with the --cleanup=space option by default, but this has been broken since late 2017. This has been fixed. Will merge to 'master'. * dr/ref-filter-push-track-fix (2019-04-18) 1 commit (merged to 'next' on 2019-04-25 at 07db067adc) + ref-filter: use correct branch for %(push:track) %(push:track) token used in the --format option to "git for-each-ref" and friends was not showing the right branch, which has been fixed. Will merge to 'master'. * cc/aix-has-fileno-as-a-macro (2019-04-25) 1 commit (merged to 'next' on 2019-04-25 at f1d6464f98) + Makefile: use fileno macro work around on AIX (this branch is used by cc/access-on-aix-workaround.) AIX shared the same build issues with other BSDs around fileno(fp), which has been corrected. Will merge to 'master'. * jk/prune-optim (2019-04-19) 1 commit (merged to 'next' on 2019-04-25 at c50353b27f) + t5304: add a test for pruning with bitmaps A follow-up test for an earlier "git prune" improvements. Will merge to 'master'. * jk/untracked-cache-more-fixes (2019-04-19) 3 commits (merged to 'next' on 2019-04-25 at a6037ddd54) + untracked-cache: simplify parsing by dropping "len" + untracked-cache: simplify parsing by dropping "next" + untracked-cache: be defensive about missing NULs in index Code clean-up. Will merge to 'master'. * jt/submodule-repo-is-with-worktree (2019-04-21) 1 commit (merged to 'next' on 2019-04-25 at da2c6d684d) + worktree: update is_bare heuristics The logic to tell if a Git repository has a working tree protects "git branch -D" from removing the branch that is currently checked out by mistake. The implementation of this logic was broken for repositories with unusual name, which unfortunately is the norm for submodules these days. This has been fixed. Will merge to 'master'. * vk/autoconf-gettext (2019-04-19) 1 commit (merged to 'next' on 2019-04-25 at 918870cbc2) + autoconf: #include <libintl.h> when checking for gettext() The autoconf generated configure script failed to use the right gettext() implementations from -libintl by ignoring useless stub implementations shipped in some C library, which has been corrected. Will merge to 'master'. * dl/no-extern-in-func-decl (2019-04-24) 4 commits - cocci: prevent extern function declarations - *.[ch]: manually align parameter lists - *.[ch]: remove extern from function declarations using sed - *.[ch]: remove extern from function declarations using spatch Mechanically and systematically drop "extern" from function declarlation. Will merge to 'next'. This is the kind of code churn patch that causes heavy conflicts with multiple topics in flight, which causes conflicts in slightly different shape when it gets merged with them in different order. I've queued it moderately early in 'pu' for the night, and I am reasonably sure the merge of the topic itself is OK, but I do not have much confidence in the resolutions of conflicts with other topics later merged to 'pu'. * js/partial-clone-connectivity-check (2019-04-21) 1 commit (merged to 'next' on 2019-04-25 at ebd8b4bffd) + clone: do faster object check for partial clones During an initial "git clone --depth=..." partial clone, it is pointless to spend cycles for a large portion of the connectivity check that enumerates and skips promisor objects (which by definition is all objects fetched from the other side). This has been optimized out. Will merge to 'master'. * hs/send-email-transferencoding-fix (2019-04-10) 1 commit - send-email: honor transferencoding config option again Since "git send-email" learned to take 'auto' as the value for the transfer-encoding, it by mistake stopped honoring the values given to the configuration variables sendemail.transferencoding and/or sendemail.<ident>.transferencoding. Attempt to correct this. Not quite. cf. <xmqq8swi34h5.fsf@gitster-ct.c.googlers.com> * jk/pack-objects-reports-num-objects-to-trace2 (2019-04-12) 1 commit (merged to 'next' on 2019-04-25 at e79464c054) + pack-objects: write objects packed to trace2 The "git pack-objects" command learned to report the number of objects it packed via the trace2 mechanism. Will merge to 'master'. * js/misc-doc-fixes (2019-04-19) 8 commits (merged to 'next' on 2019-04-25 at 6898f709d0) + Turn `git serve` into a test helper + test-tool: handle the `-C <directory>` option just like `git` + check-docs: do not bother checking for legacy scripts' documentation + docs: exclude documentation for commands that have been excluded + check-docs: allow command-list.txt to contain excluded commands + help -a: do not list commands that are excluded from the build + Makefile: drop the NO_INSTALL variable + remote-testgit: move it into the support directory for t5801 "make check-docs", "git help -a", etc. did not account for cases where a particular build may deliberately omit some subcommands, which has been corrected. Will merge to 'master'. * dl/warn-tagging-a-tag (2019-04-12) 2 commits (merged to 'next' on 2019-04-25 at 8b966d7fe8) + tag: advise on nested tags + tag: fix formatting "git tag" learned to give an advice suggesting it might be a mistake when creating an annotated or signed tag that points at another tag. Will merge to 'master'. * jc/format-patch-delay-message-id (2019-04-05) 1 commit - format-patch: move message-id and related headers to the end The location "git format-patch --thread" adds the Message-Id: header in the series of header fields has been moved down, which may help working around a suspected bug in GMail MSA, reported at <CAHk-=whP1stFZNAaJiMi5eZ9rj0MRt20Y_yHVczZPH+O01d+sA@mail.gmail.com> Waiting for feedback to see if it truly helps. Needs tests. * tb/unexpected (2019-04-10) 7 commits (merged to 'next' on 2019-04-25 at c49927fca0) + rev-list: detect broken root trees + rev-list: let traversal die when --missing is not in use + get_commit_tree(): return NULL for broken tree + list-objects.c: handle unexpected non-tree entries + list-objects.c: handle unexpected non-blob entries + t: introduce tests for unexpected object types + t: move 'hex2oct' into test-lib-functions.sh Code tightening against a "wrong" object appearing where an object of a different type is expected, instead of blindly assuming that the connection between objects are correctly made. Will merge to 'master'. * en/merge-directory-renames (2019-04-08) 15 commits (merged to 'next' on 2019-04-25 at fd5b4f57b4) + merge-recursive: switch directory rename detection default + merge-recursive: give callers of handle_content_merge() access to contents + merge-recursive: track information associated with directory renames + t6043: fix copied test description to match its purpose + merge-recursive: switch from (oid,mode) pairs to a diff_filespec + merge-recursive: cleanup handle_rename_* function signatures + merge-recursive: track branch where rename occurred in rename struct + merge-recursive: remove ren[12]_other fields from rename_conflict_info + merge-recursive: shrink rename_conflict_info + merge-recursive: move some struct declarations together + merge-recursive: use 'ci' for rename_conflict_info variable name + merge-recursive: rename locals 'o' and 'a' to 'obuf' and 'abuf' + merge-recursive: rename diff_filespec 'one' to 'o' + merge-recursive: rename merge_options argument from 'o' to 'opt' + Use 'unsigned short' for mode, like diff_filespec does "git merge-recursive" backend recently learned a new heuristics to infer file movement based on how other files in the same directory moved. As this is inherently less robust heuristics than the one based on the content similarity of the file itself (rather than based on what its neighbours are doing), it sometimes gives an outcome unexpected by the end users. This has been toned down to leave the renamed paths in higher/conflicted stages in the index so that the user can examine and confirm the result. Will merge to 'master'. * jt/clone-server-option (2019-04-18) 2 commits (merged to 'next' on 2019-04-25 at 21f07cc85d) + clone: send server options when using protocol v2 + transport: die if server options are unsupported "git clone" learned a new --server-option option when talking over the protocol version 2. Will merge to 'master'. * tz/git-svn-doc-markup-fix (2019-04-10) 1 commit (merged to 'next' on 2019-04-25 at 3efaa6285c) + Documentation/git-svn: improve asciidoctor compatibility Doc formatting fix. Will merge to 'master'. * ss/msvc-path-utils-fix (2019-04-09) 1 commit (merged to 'next' on 2019-04-25 at ee2850da18) + MSVC: include compat/win32/path-utils.h for MSVC, too, for real_path() An earlier update for MinGW and Cygwin accidentally broke MSVC build, which has been fixed. Will merge to 'master'. * cc/multi-promisor (2019-04-15) 17 commits - Move core_partial_clone_filter_default to promisor-remote.c - Move repository_format_partial_clone to promisor-remote.c - Remove fetch-object.{c,h} in favor of promisor-remote.{c,h} - remote: add promisor and partial clone config to the doc - partial-clone: add multiple remotes in the doc - t0410: test fetching from many promisor remotes - builtin/fetch: remove unique promisor remote limitation - promisor-remote: parse remote.*.partialclonefilter - diff: use promisor-remote.h instead of fetch-object.h - Use promisor_remote_get_direct() and has_promisor_remote() - promisor-remote: use repository_format_partial_clone - promisor-remote: add promisor_remote_reinit() - promisor-remote: implement promisor_remote_get_direct() - Add initial support for many promisor remotes - fetch-object: make functions return an error code - t0410: remove pipes after git commands - Merge branch 'jt/batch-fetch-blobs-in-diff' into cc/multi-promisor Teach the lazy clone machinery that there can be more than one promisor remote and consult them in order when downloading missing objects on demand. Needs review. * nd/switch-and-restore (2019-04-12) 43 commits - help: move git-diff and git-reset to different groups - rm: add --staged as alias for --cached - doc: promote "git restore" - user-manual.txt: prefer 'merge --abort' over 'reset --hard' - completion: support restore - t: add tests for restore - restore: support --patch - restore: replace --force with --ignore-unmerged - restore: default to --source=HEAD when only --staged is specified - restore: reject invalid combinations with --staged - restore: add --worktree and --staged - checkout: factor out worktree checkout code - restore: disable overlay mode by default - restore: make pathspec mandatory - restore: take tree-ish from --source option instead - checkout: split part of it to new command 'restore' - doc: promote "git switch" - completion: support switch - t: add tests for switch - switch: make --orphan switch to an empty tree - switch: reject if some operation is in progress - switch: no worktree status unless real branch switch happens - switch: implicit dwim, use --no-guess to disable it - switch: add short option for --detach - switch: only allow explicit detached HEAD - switch: reject "do nothing" case - switch: stop accepting pathspec - switch: remove -l - switch: add --discard-changes - switch: better names for -b and -B - checkout: split part of it to new command 'switch' - checkout: split options[] array in three pieces - checkout: move 'confict_style' and 'dwim_..' to checkout_opts - checkout: make "opts" in cmd_checkout() a pointer - checkout: factor out some code in parse_branchname_arg() - checkout: keep most #include sorted - checkout: inform the user when removing branch state - checkout: advice how to get out of detached HEAD mode - t: rename t2014-switch.sh to t2014-checkout-switch.sh - git-checkout.txt: fix monospace typeset - doc: document --overwrite-ignore - git-checkout.txt: fix one syntax line - git-checkout.txt: spell out --no-option Two new commands "git switch" and "git restore" are introduced to split "checking out a branch to work on advancing its history" and "checking out paths out of the index and/or a tree-ish to work on advancing the current history" out of the single "git checkout" command. The "switch" part seems more or less ready for testing. Perhaps we should split this back into two topics and merge it to 'next'. cf. <20190329103919.15642-1-pclouds@gmail.com> (switch v6) cf. <20190411131218.19195-1-pclouds@gmail.com> (restore v2) * ew/repack-with-bitmaps-by-default (2019-03-18) 3 commits - pack-objects: default to writing bitmap hash-cache - t5310: correctly remove bitmaps for jgit test - repack: enable bitmaps by default on bare repos The connectivity bitmaps are created by default in bare repositories now; also the pathname hash-cache is created by default to avoid making crappy deltas when repacking. Will merge to 'next'. * jc/format-patch-noclobber (2019-02-22) 1 commit - format-patch: --no-clobber refrains from overwriting output files "git format-patch" used to overwrite an existing patch/cover-letter file. A new "--no-clobber" option stops it. Undecided but inclined to discard. * am/p4-branches-excludes (2019-04-02) 8 commits - git-p4: respect excluded paths when detecting branches - git-p4: add failing test for "git-p4: respect excluded paths when detecting branches" - git-p4: don't exclude other files with same prefix - git-p4: add failing test for "don't exclude other files with same prefix" - git-p4: don't groom exclude path list on every commit - git-p4: match branches case insensitively if configured - git-p4: add failing test for "git-p4: match branches case insensitively if configured" - git-p4: detect/prevent infinite loop in gitCommitByP4Change() "git p4" update. Is this ready for 'next'? * cc/replace-graft-peel-tags (2019-04-15) 4 commits (merged to 'next' on 2019-04-25 at f8d0db27ba) + replace: peel tag when passing a tag first to --graft + replace: peel tag when passing a tag as parent to --graft + t6050: redirect expected error output to a file + t6050: use test_line_count instead of wc -l When given a tag that points at a commit-ish, "git replace --graft" failed to peel the tag before writing a replace ref, which did not make sense because the old graft mechanism the feature wants to mimick only allowed to replace one commit object with another. This has been fixed. Will merge to 'master'. * dl/rebase-i-keep-base (2019-04-25) 6 commits - rebase: teach rebase --keep-base - rebase: fast-forward --fork-point in more cases - rebase: fast-forward --onto in more cases - rebase: refactor can_fast_forward into goto tower - t3432: test rebase fast-forward behavior - t3431: add rebase --fork-point tests "git rebase --keep-base <upstream>" tries to find the original base of the topic being rebased and rebase on top of that same base, which is useful when running the "git rebase -i" (and its limited variant "git rebase -x"). The command also has learned to fast-forward in more cases where it can instead of replaying to recreate identical commits. Will merge to 'next'. * jh/trace2-sid-fix (2019-04-16) 10 commits (merged to 'next' on 2019-04-25 at a5c08f1226) + trace2: update docs to describe system/global config settings + trace2: make SIDs more unique + trace2: clarify UTC datetime formatting + trace2: report peak memory usage of the process + trace2: use system/global config for default trace2 settings + config: add read_very_early_config() + trace2: find exec-dir before trace2 initialization + trace2: add absolute elapsed time to start event + trace2: refactor setting process starting time + config: initialize opts structure in repo_read_config() Polishing of the new trace2 facility continues. The system-level configuration can specify site-wide trace2 settings, which can be overridden with per-user configuration and environment variables. Will merge to 'master'. cf. <pull.169.v4.git.gitgitgadget@gmail.com> (v4) * js/trace2-to-directory (2019-03-22) 1 commit (merged to 'next' on 2019-04-25 at 53adf71c41) + trace2: write to directory targets The trace2 tracing facility learned to auto-generate a filename when told to log to a directory. Will merge to 'master'. * km/empty-repo-is-still-a-repo (2019-04-10) 3 commits (merged to 'next' on 2019-04-25 at bb3d4406a5) + add: error appropriately on repository with no commits + dir: do not traverse repositories with no commits + submodule: refuse to add repository with no commits Running "git add" on a repository created inside the current repository is an explicit indication that the user wants to add it as a submodule, but when the HEAD of the inner repository is on an unborn branch, it cannot be added as a submodule. Worse, the files in its working tree can be added as if they are a part of the outer repository, which is not what the user wants. These problems are being addressed. Will merge to 'master'. * nd/precious (2019-04-09) 1 commit - Introduce "precious" file concept "git clean" learned to pay attention to the 'precious' attributes and keep untracked paths with the attribute instead of removing when the "--keep-precious" is given. Retracted. cf. <CACsJy8AEZ-Lz6zgEsuNukvphB9TTa9FAC1gK05fhnie2xtfc9w@mail.gmail.com> I am not sure what aspect of this longer-term "precious" vision, which gets taught to various commands and use cases individually and incrementally, Ævar finds problematic, which I understand is the reason of redtraction. * nd/sha1-name-c-wo-the-repository (2019-04-16) 34 commits (merged to 'next' on 2019-04-25 at d826918329) + sha1-name.c: remove the_repo from get_oid_mb() + sha1-name.c: remove the_repo from other get_oid_* + sha1-name.c: remove the_repo from maybe_die_on_misspelt_object_name + submodule-config.c: use repo_get_oid for reading .gitmodules + sha1-name.c: add repo_get_oid() + sha1-name.c: remove the_repo from get_oid_with_context_1() + sha1-name.c: remove the_repo from resolve_relative_path() + sha1-name.c: remove the_repo from diagnose_invalid_index_path() + sha1-name.c: remove the_repo from handle_one_ref() + sha1-name.c: remove the_repo from get_oid_1() + sha1-name.c: remove the_repo from get_oid_basic() + sha1-name.c: remove the_repo from get_describe_name() + sha1-name.c: remove the_repo from get_oid_oneline() + sha1-name.c: add repo_interpret_branch_name() + sha1-name.c: remove the_repo from interpret_branch_mark() + sha1-name.c: remove the_repo from interpret_nth_prior_checkout() + sha1-name.c: remove the_repo from get_short_oid() + sha1-name.c: add repo_for_each_abbrev() + sha1-name.c: store and use repo in struct disambiguate_state + sha1-name.c: add repo_find_unique_abbrev_r() + sha1-name.c: remove the_repo from find_abbrev_len_packed() + sha1-name.c: remove the_repo from sort_ambiguous() + commit.c: add repo_get_commit_tree() + commit.cocci: refactor code, avoid double rewrite + refs.c: remove the_repo from read_ref_at() + refs.c: add repo_dwim_log() + refs.c: add repo_dwim_ref() + refs.c: remove the_repo from expand_ref() + refs.c: remove the_repo from substitute_branch_name() + refs.c: add refs_shorten_unambiguous_ref() + refs.c: add refs_ref_exists() + packfile.c: add repo_approximate_object_count() + builtin rebase: use oideq() + builtin rebase: use FREE_AND_NULL Further code clean-up to allow the lowest level of name-to-object mapping layer to work with a passed-in repository other than the default one. Will merge to 'master'. * pw/rebase-i-internal (2019-04-19) 13 commits - rebase -i: run without forking rebase--interactive - rebase: use a common action enum - rebase -i: use struct rebase_options in do_interactive_rebase() - rebase -i: use struct rebase_options to parse args - rebase -i: use struct object_id for squash_onto - rebase -i: use struct commit when parsing options - rebase -i: remove duplication - rebase -i: combine rebase--interactive.c with rebase.c - rebase: use OPT_RERERE_AUTOUPDATE() - rebase: rename write_basic_state() - rebase: don't translate trace strings - sequencer: always discard index after checkout - Merge branch 'ag/sequencer-reduce-rewriting-todo' into pw/rebase-i-internal The internal implementation of "git rebase -i" has been updated to avoid forking a separate "rebase--interactive" process. Will merge to 'next'. * nd/worktree-name-sanitization (2019-03-20) 2 commits - SQUASH??? - worktree add: sanitize worktree names In recent versions of Git, per-worktree refs are exposed in refs/worktrees/<wtname>/ hierarchy, which means that worktree names must be a valid refname component. The code now sanitizes the names given to worktrees, to make sure these refs are well-formed. I am inclined to squash the fix at the tip in and merge the result to 'next'. Opinions? * dm/some-stdio-functions-are-macro-on-freebsd (2019-02-01) 1 commit - http: cast result to FILE * Variants of BSD define fileno(fh) as a macro, breaking a program that passes a "void *" to it. Expecting a reroll. cf. <49B9198C-53E5-42BD-8834-B1EDEB3332CB@usask.ca> * ds/commit-graph-format-v2 (2019-04-25) 5 commits - commit-graph: implement file format version 2 - commit-graph: add --version=<n> option - commit-graph: create new version flags - commit-graph: collapse parameters into flags - commit-graph: return with errors during write Introduce version 2 of the commit-graph format to correct deficiency in the initial version. cf. <pull.112.v2.git.gitgitgadget@gmail.com> (v2) * br/blame-ignore (2019-04-14) 6 commits - blame: use a fingerprint heuristic to match ignored lines - blame: optionally track line fingerprints during fill_blame_origin() - blame: add config options to handle output for ignored lines - blame: add the ability to ignore commits and their changes - blame: use a helper function in blame_chunk() - Move init_skiplist() outside of fsck "git blame" learned to "ignore" commits in the history, whose effects (as well as their presence) get ignored. Expecting a reroll. cf. <20190410162409.117264-1-brho@google.com> (v6) cf. <a742dd62-c84e-1f85-0663-4a3aa4d14989@google.com> cf. <3db6bad3-e7a5-af1d-3fe2-321bd17db2c6@google.com> * nd/config-move-to (2019-01-14) 7 commits - config.h: fix hdr-check warnings - config: add --move-to - config: factor out set_config_source_file() - config: use OPT_FILENAME() - config.c: add repo_config_set_worktree_gently() - worktree.c: add get_worktree_config() - config.c: avoid git_path() in do_git_config_sequence() Needs review. * dl/merge-cleanup-scissors-fix (2019-04-19) 10 commits (merged to 'next' on 2019-04-25 at 2014eef6b1) + cherry-pick/revert: add scissors line on merge conflict + sequencer.c: save and restore cleanup mode + merge: add scissors line on merge conflict + merge: cleanup messages like commit + parse-options.h: extract common --cleanup option + commit: extract cleanup_mode functions to sequencer + t7502: clean up style + t7604: clean up style + t3507: clean up style + t7600: clean up style (this branch uses pw/sequencer-cleanup-with-signoff-x-fix.) The list of conflicted paths shown in the editor while concluding a conflicted merge was shown above the scissors line when the clean-up mode is set to "scissors", even though it was commented out just like the list of updated paths and other information to help the user explain the merge better. Will merge to 'master'. * jn/unknown-index-extensions (2018-11-21) 2 commits - index: offer advice for unknown index extensions - index: do not warn about unrecognized extensions A bit too alarming warning given when unknown index extensions exist is getting revamped. Expecting a reroll. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano @ 2019-04-25 14:50 ` Elijah Newren 2019-04-25 22:16 ` js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) Josh Steadmon ` (3 subsequent siblings) 4 siblings, 0 replies; 38+ messages in thread From: Elijah Newren @ 2019-04-25 14:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Thu, Apr 25, 2019 at 7:16 AM Junio C Hamano <gitster@pobox.com> wrote > * en/unicode-in-refnames (2019-04-24) 1 commit > - Honor core.precomposeUnicode in more places > > The names of the refs stored as filesystem entities may become > different from what the end-user expects, just like files in the > working tree gets "renamed", on a filesystem like HFS+. Work it > around by paying attemption to the core.precomposeUnicode > configuration. > > Looked sensible. Ready for next? Almost. Torsten pointed out that I left some commentary in the commit message that I should remove. I'll send a v2 with it excised. Also, s/it around/around it/ and s/attemption/attention/ in your summary? > * nd/switch-and-restore (2019-04-12) 43 commits > - help: move git-diff and git-reset to different groups > - rm: add --staged as alias for --cached > - doc: promote "git restore" > - user-manual.txt: prefer 'merge --abort' over 'reset --hard' > - completion: support restore > - t: add tests for restore > - restore: support --patch > - restore: replace --force with --ignore-unmerged > - restore: default to --source=HEAD when only --staged is specified > - restore: reject invalid combinations with --staged > - restore: add --worktree and --staged > - checkout: factor out worktree checkout code > - restore: disable overlay mode by default > - restore: make pathspec mandatory > - restore: take tree-ish from --source option instead > - checkout: split part of it to new command 'restore' > - doc: promote "git switch" > - completion: support switch > - t: add tests for switch > - switch: make --orphan switch to an empty tree > - switch: reject if some operation is in progress > - switch: no worktree status unless real branch switch happens > - switch: implicit dwim, use --no-guess to disable it > - switch: add short option for --detach > - switch: only allow explicit detached HEAD > - switch: reject "do nothing" case > - switch: stop accepting pathspec > - switch: remove -l > - switch: add --discard-changes > - switch: better names for -b and -B > - checkout: split part of it to new command 'switch' > - checkout: split options[] array in three pieces > - checkout: move 'confict_style' and 'dwim_..' to checkout_opts > - checkout: make "opts" in cmd_checkout() a pointer > - checkout: factor out some code in parse_branchname_arg() > - checkout: keep most #include sorted > - checkout: inform the user when removing branch state > - checkout: advice how to get out of detached HEAD mode > - t: rename t2014-switch.sh to t2014-checkout-switch.sh > - git-checkout.txt: fix monospace typeset > - doc: document --overwrite-ignore > - git-checkout.txt: fix one syntax line > - git-checkout.txt: spell out --no-option > > Two new commands "git switch" and "git restore" are introduced to > split "checking out a branch to work on advancing its history" and > "checking out paths out of the index and/or a tree-ish to work on > advancing the current history" out of the single "git checkout" > command. > > The "switch" part seems more or less ready for testing. Perhaps > we should split this back into two topics and merge it to 'next'. > cf. <20190329103919.15642-1-pclouds@gmail.com> (switch v6) > cf. <20190411131218.19195-1-pclouds@gmail.com> (restore v2) Splitting apart and advancing "switch" makes sense to me. I'm behind on stuff but really need to take a look at the new restore series; I'll try to get to that soon. ^ permalink raw reply [flat|nested] 38+ messages in thread
* js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano 2019-04-25 14:50 ` Elijah Newren @ 2019-04-25 22:16 ` Josh Steadmon 2019-05-02 2:52 ` Jeff King 2019-04-26 5:05 ` What's cooking in git.git (Apr 2019, #05; Thu, 25) Taylor Blau ` (2 subsequent siblings) 4 siblings, 1 reply; 38+ messages in thread From: Josh Steadmon @ 2019-04-25 22:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff On 2019.04.25 19:15, Junio C Hamano wrote: > * js/partial-clone-connectivity-check (2019-04-21) 1 commit > (merged to 'next' on 2019-04-25 at ebd8b4bffd) > + clone: do faster object check for partial clones > > During an initial "git clone --depth=..." partial clone, it is > pointless to spend cycles for a large portion of the connectivity > check that enumerates and skips promisor objects (which by > definition is all objects fetched from the other side). This has > been optimized out. > > Will merge to 'master'. Peff asked for a perf test for this [1], but I haven't had time to write one yet. I can do that in a separate patch if you still want to merge this as-is. [1]: https://public-inbox.org/git/20190422213113.GB4728@sigill.intra.peff.net/ ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) 2019-04-25 22:16 ` js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) Josh Steadmon @ 2019-05-02 2:52 ` Jeff King 2019-05-02 16:48 ` Josh Steadmon 2019-05-02 21:45 ` Jeff King 0 siblings, 2 replies; 38+ messages in thread From: Jeff King @ 2019-05-02 2:52 UTC (permalink / raw) To: Josh Steadmon; +Cc: Junio C Hamano, git On Thu, Apr 25, 2019 at 03:16:57PM -0700, Josh Steadmon wrote: > On 2019.04.25 19:15, Junio C Hamano wrote: > > * js/partial-clone-connectivity-check (2019-04-21) 1 commit > > (merged to 'next' on 2019-04-25 at ebd8b4bffd) > > + clone: do faster object check for partial clones > > > > During an initial "git clone --depth=..." partial clone, it is > > pointless to spend cycles for a large portion of the connectivity > > check that enumerates and skips promisor objects (which by > > definition is all objects fetched from the other side). This has > > been optimized out. > > > > Will merge to 'master'. > > Peff asked for a perf test for this [1], but I haven't had time to write one > yet. I can do that in a separate patch if you still want to merge this > as-is. I won't die without one, but it would be nice. It may also be that an existing perf test, but I don't think we cover partial clones in t/perf at all. Might be worth just a straight-up "git clone --filter=blob:none" test. Also, in the proposed merge message above, it should be --filter, not --depth, right? -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) 2019-05-02 2:52 ` Jeff King @ 2019-05-02 16:48 ` Josh Steadmon 2019-05-02 21:45 ` Jeff King 1 sibling, 0 replies; 38+ messages in thread From: Josh Steadmon @ 2019-05-02 16:48 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git On 2019.05.01 22:52, Jeff King wrote: > On Thu, Apr 25, 2019 at 03:16:57PM -0700, Josh Steadmon wrote: > > > On 2019.04.25 19:15, Junio C Hamano wrote: > > > * js/partial-clone-connectivity-check (2019-04-21) 1 commit > > > (merged to 'next' on 2019-04-25 at ebd8b4bffd) > > > + clone: do faster object check for partial clones > > > > > > During an initial "git clone --depth=..." partial clone, it is > > > pointless to spend cycles for a large portion of the connectivity > > > check that enumerates and skips promisor objects (which by > > > definition is all objects fetched from the other side). This has > > > been optimized out. > > > > > > Will merge to 'master'. > > > > Peff asked for a perf test for this [1], but I haven't had time to write one > > yet. I can do that in a separate patch if you still want to merge this > > as-is. > > I won't die without one, but it would be nice. It may also be that an > existing perf test, but I don't think we cover partial clones in t/perf > at all. Might be worth just a straight-up "git clone --filter=blob:none" > test. > > Also, in the proposed merge message above, it should be --filter, not > --depth, right? Yes, thanks for the catch. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) 2019-05-02 2:52 ` Jeff King 2019-05-02 16:48 ` Josh Steadmon @ 2019-05-02 21:45 ` Jeff King 2019-05-02 22:24 ` Jeff King 1 sibling, 1 reply; 38+ messages in thread From: Jeff King @ 2019-05-02 21:45 UTC (permalink / raw) To: Josh Steadmon; +Cc: Junio C Hamano, git On Wed, May 01, 2019 at 10:52:06PM -0400, Jeff King wrote: > > > * js/partial-clone-connectivity-check (2019-04-21) 1 commit > > > (merged to 'next' on 2019-04-25 at ebd8b4bffd) > > > + clone: do faster object check for partial clones > > > > > > During an initial "git clone --depth=..." partial clone, it is > > > pointless to spend cycles for a large portion of the connectivity > > > check that enumerates and skips promisor objects (which by > > > definition is all objects fetched from the other side). This has > > > been optimized out. > > > > > > Will merge to 'master'. > > > > Peff asked for a perf test for this [1], but I haven't had time to write one > > yet. I can do that in a separate patch if you still want to merge this > > as-is. > > I won't die without one, but it would be nice. It may also be that an > existing perf test, but I don't think we cover partial clones in t/perf > at all. Might be worth just a straight-up "git clone --filter=blob:none" > test. Here's what I came up with. Note that there's a bug in 'master' right now which causes perf to produce nonsense results. It's due to my 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling, 2019-03-15). I'll fix that separately (the timing below is done with that commit reverted). -- >8 -- Subject: [PATCH] t/perf: add perf script for partial clones We don't cover the partial clone feature at all in t/perf. Let's at least run a few basic tests so that we'll notice any regressions. We'll do a no-blob clone, and split it into two parts: the actual object transfer, and the subsequent checkout (which will of course require another transfer to get the blobs). That will help us more clearly assess the performance of each. There are obviously a lot more possibilities besides just a no-blob partial clone, but this should serve as a canary that alerts us to any generic slow-downs (and we can add more tests later for cases that aren't exercised here). There are a few non-ideal things here that make this not an entirely accurate test, but are probably OK for our purposes: 1. We have to do some extra prep/cleanup work inside the timing tests, since they impact the on-disk state and the perf harness may run each one multiple times. In practice this is probably OK, since these bits should be much less expensive than the operations we are measuring. 2. The clone time is likely to be dominated by the server's object enumeration. In the real world, a repo large enough to drive people to partial clones is likely to have reachability bitmaps enabled. And in the opposite direction, our object transfer is happening at the speed of a local pipe, whereas in the real world it would bottle-neck on the network. So any percentage speedups should be taken with a grain of salt. But hopefully any regressions will produce enough of an effect to be noticeable. This script also demonstrates the recent improvement from dfa33a298d (clone: do faster object check for partial clones, 2019-04-19): Test dfa33a298d^ dfa33a298d ------------------------------------------------------------------------- 5600.2: clone without blobs 18.41(22.72+1.09) 6.83(11.65+0.50) -62.9% 5600.3: checkout of result 1.82(3.24+0.26) 1.84(3.24+0.26) +1.1% Signed-off-by: Jeff King <peff@peff.net> --- The speedup from dfa33a298d was larger than I expected. Doing a full `rev-list --objects --all` on a non-partial clone is only 3-4s. So how did we manage to save 11s by removing it? The answer is that the is_promisor check is super expensive: we have to walk all of those trees in the partial pack to find which blobs are mentioned, only to then walk them all again for the actual traversal and say "yep, this is in our promisor list". Yikes. Your patch makes all of that go away, but I do think there's room for us to be more clever here (but that leads us back down the rabbit hole you explored earlier, so I think it makes sense to take your patch now and deal with other optimizations separately). t/perf/p5600-partial-clone.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100755 t/perf/p5600-partial-clone.sh diff --git a/t/perf/p5600-partial-clone.sh b/t/perf/p5600-partial-clone.sh new file mode 100755 index 0000000000..3e04bd2ae1 --- /dev/null +++ b/t/perf/p5600-partial-clone.sh @@ -0,0 +1,26 @@ +#!/bin/sh + +test_description='performance of partial clones' +. ./perf-lib.sh + +test_perf_default_repo + +test_expect_success 'enable server-side config' ' + git config uploadpack.allowFilter true && + git config uploadpack.allowAnySHA1InWant true +' + +test_perf 'clone without blobs' ' + rm -rf bare.git && + git clone --no-local --bare --filter=blob:none . bare.git +' + +test_perf 'checkout of result' ' + rm -rf worktree && + mkdir -p worktree/.git && + tar -C bare.git -cf - . | tar -C worktree/.git -xf - && + git -C worktree config core.bare false && + git -C worktree checkout -f +' + +test_done -- 2.21.0.1314.g224b191707 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) 2019-05-02 21:45 ` Jeff King @ 2019-05-02 22:24 ` Jeff King 2019-05-06 19:16 ` [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 38+ messages in thread From: Jeff King @ 2019-05-02 22:24 UTC (permalink / raw) To: Josh Steadmon; +Cc: Junio C Hamano, git On Thu, May 02, 2019 at 05:45:09PM -0400, Jeff King wrote: > Here's what I came up with. Note that there's a bug in 'master' right > now which causes perf to produce nonsense results. It's due to my > 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling, > 2019-03-15). I'll fix that separately (the timing below is done with > that commit reverted). And here's the fix for that. It's rather subtle, so I hope I explained it sufficiently. I didn't notice it while working on the original because everything _appears_ to run fine, but you just get timings from the wrong version of Git. Which is only noticeable if you're literally testing two versions that you expect to differ. -- >8 -- Subject: [PATCH] t/perf: set GIT_TEST_INSTALLED before including test-lib.sh Commit 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling, 2019-03-15) introduced a bug which causes perf test runs to always find git in the $PATH, rather than actually testing $GIT_TEST_INSTALLED (which is used by the "run" script). That commit bumped the conversion of $GIT_TEST_INSTALLED to an absolute path until after test-lib.sh was been included. That solved the original problem of generating a bogus $perf_results_prefix value from the absolute path. But it introduced a new one: test-lib.sh needs to see that absolute value because it will put it at the front of the $PATH. The root of the issue is that we need both the relative and the absolute paths available. That earlier commit chose to stuff the absolute path into $ABSOLUTE_GIT_TEST_INSTALLED, keep the relative one in $GIT_TEST_INSTALLED, and then resolve the two after test-lib.sh has been loaded. We can fix it by reversing our strategy: we'll keep the absolute path in $GIT_TEST_INSTALLED, drop our now-useless ABSOLUTE variable, and introduce a new $TEST_INSTALLED_ORIG variable to hold the original value (from which we will later derive the prefix). Note that we need to export our ORIG variable because the whole point of 0baf78e7bc is that test-lib.sh may actually re-exec a new copy of the script to handle tee-ing the output. I'd hoped to add a test to p0000-perf-lib-sanity to cover this, but the bug is actually outside that scope. It triggers if you do something like: cat >t/perf/p1234-foo.sh <<-\EOF #!/bin/sh test_description=debugging . ./perf-lib.sh test_expect_success 'version' 'which git && git version' test_perf 'fake timing' 'true' test_done EOF chmod +x t/perf/p1234-foo.sh make GIT_TEST_OPTS=--verbose-log cd t/perf ./run origin p1234-foo.sh cat test-results/p1234-foo.out Without this patch, you'll see that we're running whatever git is in your $PATH, not the one from "origin". With this patch, we should see: - the correct git is run - we get timing results out of ./run, showing that the prefix bug from 0baf78e7bc was not regressed - our values make it across the script re-exec due to --verbose-log Signed-off-by: Jeff King <peff@peff.net> --- t/perf/perf-lib.sh | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..8317dbef44 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -21,7 +21,11 @@ # because it will change our working directory. TEST_DIRECTORY=$(pwd)/.. TEST_OUTPUT_DIRECTORY=$(pwd) -ABSOLUTE_GIT_TEST_INSTALLED=$( + +# Likewise we must turn GIT_TEST_INSTALLED into an absolute path; but remember +# and export the original value, since we'll later generate our prefix from it. +: ${TEST_INSTALLED_ORIG=$GIT_TEST_INSTALLED}; export TEST_INSTALLED_ORIG +GIT_TEST_INSTALLED=$( test -n "$GIT_TEST_INSTALLED" && cd "$GIT_TEST_INSTALLED" && pwd) TEST_NO_CREATE_REPO=t @@ -32,8 +36,7 @@ TEST_NO_MALLOC_CHECK=t if test -z "$GIT_TEST_INSTALLED"; then perf_results_prefix= else - perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." - GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED + perf_results_prefix=$(printf "%s" "${TEST_INSTALLED_ORIG%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." fi # Variables from test-lib that are normally internal to the tests; we -- 2.21.0.1314.g224b191707 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits 2019-05-02 22:24 ` Jeff King @ 2019-05-06 19:16 ` Ævar Arnfjörð Bjarmason 2019-05-06 20:24 ` Jeff King 0 siblings, 1 reply; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-06 19:16 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason Fix a really bad regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15). Since that change all runs of different <revisions> of git have used the git found in the user's $PATH, e.g. /usr/bin/git instead of the <revision> we just built and wanted to performance test. The problem starts with GIT_TEST_INSTALLED not working like our non-perf tests with the "run" script. I.e. you can't run performance tests against a given installed git. Instead we expect to use it ourselves to point GIT_TEST_INSTALLED to the <revision> we just built. However, we had been relying on '$(cd "$GIT_TEST_INSTALLED" && pwd)' to resolve that relative $GIT_TEST_INSTALLED to an absolute path *before* test-lib.sh was loaded, in cases where it was e.g. "build/<rev>/bin-wrappers" and we wanted "<abs_path>build/...". Perhaps there's some better way to fix this, but it seems to me that the best solution is to just make this behavior less magical. We know in run_dirs_helper() that we're about to run performance tests on a given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute path there, and then make getting logging target from a previously relative path less magical, we'll just explicitly pass down the relative path as a variable. This makes e.g. these cases all work: ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> As well as just a plain one-off: ./run <tests> And, since we're passing down the new GIT_PERF_DIR_MYDIR_REL we make sure the bug relating to aggregate.perl not finding our files as described in 0baf78e7bc doesn't happen again. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- On Fri, May 03 2019, Jeff King wrote: > On Thu, May 02, 2019 at 05:45:09PM -0400, Jeff King wrote: > >> Here's what I came up with. Note that there's a bug in 'master' right >> now which causes perf to produce nonsense results. It's due to my >> 0baf78e7bc (perf-lib.sh: rely on test-lib.sh for --tee handling, >> 2019-03-15). I'll fix that separately (the timing below is done with >> that commit reverted). > > And here's the fix for that. It's rather subtle, so I hope I explained > it sufficiently. I didn't notice it while working on the original > because everything _appears_ to run fine, but you just get timings from > the wrong version of Git. Which is only noticeable if you're literally > testing two versions that you expect to differ. I ran into this today and it took me an embarrasingly long time to figure out why my code wasn't making things faster. So I wrote this up before seeing your patch, since it wasn't queued in "pu" and my naïve ML search didn't include inline patches (again, *sigh*). Anyway, I wonder if something closer to this patch, or some sort of merge of the two (e.g. to still get rid of the ABSOLUTE_GIT_TEST_INSTALLED variable) is better. I.e. why try to magically detect all of this in perf-lib.sh itself, we know we're going to invoke it like this in the "run" script, so we can just set the appropriate variables there instead of this hard-to-explain magic of $GIT_TEST_INSTALLED being one value the first time, but another one the second time around. t/perf/perf-lib.sh | 4 ++++ t/perf/run | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..b15ee1d262 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t if test -z "$GIT_TEST_INSTALLED"; then perf_results_prefix= else + if test -n "$GIT_PERF_DIR_MYDIR_REL" + then + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL + fi perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED fi diff --git a/t/perf/run b/t/perf/run index 9aaa733c77..0a7c8744ab 100755 --- a/t/perf/run +++ b/t/perf/run @@ -91,10 +91,14 @@ run_dirs_helper () { if test "$mydir" = .; then unset GIT_TEST_INSTALLED else - GIT_TEST_INSTALLED="$mydir/bin-wrappers" + GIT_PERF_DIR_MYDIR_REL=$mydir + GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) + export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS + + GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" # Older versions of git lacked bin-wrappers; fallback to the # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir + test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS export GIT_TEST_INSTALLED fi run_one_dir "$@" -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits 2019-05-06 19:16 ` [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason @ 2019-05-06 20:24 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (2 more replies) 0 siblings, 3 replies; 38+ messages in thread From: Jeff King @ 2019-05-06 20:24 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano On Mon, May 06, 2019 at 09:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote: > Perhaps there's some better way to fix this, but it seems to me that > the best solution is to just make this behavior less magical. We know > in run_dirs_helper() that we're about to run performance tests on a > given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute > path there, and then make getting logging target from a previously > relative path less magical, we'll just explicitly pass down the > relative path as a variable. > > This makes e.g. these cases all work: > > ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> > > As well as just a plain one-off: > > ./run <tests> Doing this naively would break anybody doing: GIT_TEST_INSTALLED=some-relative-path ./p1234-foo.sh but I doubt that actually matters in practice (notably this already does not work with non-perf tests, as test-lib.sh does not do any normalization). I don't think your patch does, because it leaves the extra absolutizing in perf-lib.sh. But then it doesn't feel like it's really simplified anything. ;) > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh > index 169f92eae3..b15ee1d262 100644 > --- a/t/perf/perf-lib.sh > +++ b/t/perf/perf-lib.sh > @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t > if test -z "$GIT_TEST_INSTALLED"; then > perf_results_prefix= > else > + if test -n "$GIT_PERF_DIR_MYDIR_REL" > + then > + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL > + fi > perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." > GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED > fi So we reset GIT_TEST_INSTALLED to the relative path here (ignoring what's in it!), and then afterwards set it to the absolute path. That still seems rather magical. :) What if instead we: - taught test-lib.sh to make GIT_TEST_INSTALLED absolute (since after all it is the one who is planning to chdir and wreck the relative path) - let callers pass in $GIT_PERF_RESULTS_PREFIX instead of guessing at it ourselves from the path name. Then the "run" script could quite reasonably just pass in the tree oid it already has instead of us trying to decode it. And nobody would care about whether $GIT_TEST_INSTALLED has been mangled. I thought about going this route for my original patch, but I wanted to fix the regression (which I agree is quite serious and embarrassing) as quickly and simply as possible. -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs 2019-05-06 20:24 ` Jeff King @ 2019-05-06 23:23 ` Ævar Arnfjörð Bjarmason 2019-05-07 7:06 ` Jeff King ` (7 more replies) 2019-05-06 23:23 ` [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason 2019-05-06 23:23 ` [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason 2 siblings, 8 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-06 23:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason On Mon, May 06 2019, Jeff King wrote: > On Mon, May 06, 2019 at 09:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Perhaps there's some better way to fix this, but it seems to me that >> the best solution is to just make this behavior less magical. We know >> in run_dirs_helper() that we're about to run performance tests on a >> given <revision>, so let's just set GIT_TEST_INSTALLED to an absolute >> path there, and then make getting logging target from a previously >> relative path less magical, we'll just explicitly pass down the >> relative path as a variable. >> >> This makes e.g. these cases all work: >> >> ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> >> >> As well as just a plain one-off: >> >> ./run <tests> > > Doing this naively would break anybody doing: > > GIT_TEST_INSTALLED=some-relative-path ./p1234-foo.sh > > but I doubt that actually matters in practice (notably this already does > not work with non-perf tests, as test-lib.sh does not do any > normalization). Yeah this has never worked and GIT_TEST_INSTALLED has been documented to take an absolute path when used with the normal test suite. I've noted as much in the 1/2 here. > I don't think your patch does, because it leaves the extra absolutizing > in perf-lib.sh. But then it doesn't feel like it's really simplified > anything. ;) Yeah, v1 didn't because I was trying for a minimal regression fix without digging us fully out of that GIT_TEST_INSTALLED hole, but as 2/2 here shows we can fully get rid of it from perf-lib.sh, which I think makes things much simpler. >> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh >> index 169f92eae3..b15ee1d262 100644 >> --- a/t/perf/perf-lib.sh >> +++ b/t/perf/perf-lib.sh >> @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t >> if test -z "$GIT_TEST_INSTALLED"; then >> perf_results_prefix= >> else >> + if test -n "$GIT_PERF_DIR_MYDIR_REL" >> + then >> + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL >> + fi >> perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." >> GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED >> fi > > So we reset GIT_TEST_INSTALLED to the relative path here (ignoring > what's in it!), and then afterwards set it to the absolute path. That > still seems rather magical. :) > > What if instead we: > > - taught test-lib.sh to make GIT_TEST_INSTALLED absolute (since after > all it is the one who is planning to chdir and wreck the relative > path) > > - let callers pass in $GIT_PERF_RESULTS_PREFIX instead of guessing at > it ourselves from the path name. Then the "run" script could quite > reasonably just pass in the tree oid it already has instead of us > trying to decode it. And nobody would care about whether > $GIT_TEST_INSTALLED has been mangled. > > I thought about going this route for my original patch, but I wanted to > fix the regression (which I agree is quite serious and embarrassing) as > quickly and simply as possible. It seems simplest after the changes I've made here to just make a relative GIT_TEST_INSTALLED be an error in test-lib.sh, why bend over backwards to support it? Re GIT_PERF_RESULTS_PREFIX: Depending on what you mean we now have that in 2/2 as PERF_RESULTS_PREFIX. But if you mean the user can pass it in that doesn't make sense, since we need to pick a different prefix revision we test, so it's a many-to-many relationship. As seen in 2/2 modifying some of the shell & associated Perl code it's a bit nasty that we need to duplicate the logic for picking these PERF_RESULTS_PREFIXes in the shell code and Perl code. Ideally the shell code would pick it, and pass the mapping to the Perl code somehow. But that's another "has sucked since forever, future TODO" item. Ævar Arnfjörð Bjarmason (2): perf-lib.sh: make "./run <revisions>" use the correct gits perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh t/perf/aggregate.perl | 13 ++++++------- t/perf/perf-lib.sh | 11 +---------- t/perf/run | 41 +++++++++++++++++++++++++++++++---------- 3 files changed, 38 insertions(+), 27 deletions(-) -- 2.21.0.593.g511ec345e18 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason @ 2019-05-07 7:06 ` Jeff King 2019-05-07 10:54 ` [PATCH v3 0/6] " Ævar Arnfjörð Bjarmason ` (6 subsequent siblings) 7 siblings, 0 replies; 38+ messages in thread From: Jeff King @ 2019-05-07 7:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Christian Couder On Tue, May 07, 2019 at 01:23:07AM +0200, Ævar Arnfjörð Bjarmason wrote: > > What if instead we: > > > > - taught test-lib.sh to make GIT_TEST_INSTALLED absolute (since after > > all it is the one who is planning to chdir and wreck the relative > > path) > > > > - let callers pass in $GIT_PERF_RESULTS_PREFIX instead of guessing at > > it ourselves from the path name. Then the "run" script could quite > > reasonably just pass in the tree oid it already has instead of us > > trying to decode it. And nobody would care about whether > > $GIT_TEST_INSTALLED has been mangled. > > > > I thought about going this route for my original patch, but I wanted to > > fix the regression (which I agree is quite serious and embarrassing) as > > quickly and simply as possible. > > It seems simplest after the changes I've made here to just make a > relative GIT_TEST_INSTALLED be an error in test-lib.sh, why bend over > backwards to support it? I'm OK with that, though _somebody_ has to generate the full path. My thinking was that it would be a little nicer if we did at the lowest level, but it's pretty easy to do it directly in the "run" script. > Re GIT_PERF_RESULTS_PREFIX: Depending on what you mean we now have > that in 2/2 as PERF_RESULTS_PREFIX. > > But if you mean the user can pass it in that doesn't make sense, since > we need to pick a different prefix revision we test, so it's a > many-to-many relationship. I couldn't quite parse this. What I meant was that any caller of the perf scripts would pass in two pieces of data: the path to the git we want to test (GIT_TEST_INSTALLED) and the unique name under which it should be saved (GIT_PERF_PREFIX, which would generally be either empty or the tree oid). I see that perf-lib.sh also has support for running git out of some arbitrary installed directory. I'm not sure if anybody actually uses that, but it's not too hard to support either (the run script just generates the prefix). Something like (just for illustration on top of master; this doesn't have the actual bugfix): diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..e2b342f216 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -29,10 +29,7 @@ TEST_NO_MALLOC_CHECK=t . ../test-lib.sh -if test -z "$GIT_TEST_INSTALLED"; then - perf_results_prefix= -else - perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." +if test -n "$GIT_TEST_INSTALLED"; then GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED fi @@ -179,7 +176,8 @@ test_wrapper_ () { base=$(basename "$0" .sh) echo "$test_count" >>"$perf_results_dir"/$base.subtests echo "$1" >"$perf_results_dir"/$base.$test_count.descr - base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count" + prefix=$PERF_RESULTS_PREFIX${PERF_RESULTS_PREFIX:+.} + base="$perf_results_dir"/"$prefix$(basename "$0" .sh)"."$test_count" "$test_wrapper_func_" "$@" fi diff --git a/t/perf/run b/t/perf/run index 9aaa733c77..e5695ec8de 100755 --- a/t/perf/run +++ b/t/perf/run @@ -87,15 +87,20 @@ run_dirs_helper () { fi build_git_rev $rev "$mydir" mydir=build/$rev + PERF_RESULTS_PREFIX=$rev + else + PERF_RESULTS_PREFIX=$(echo mydir | tr -c "[a-zA-Z0-9]" "[_*]")"." fi if test "$mydir" = .; then unset GIT_TEST_INSTALLED + unset PERF_RESULTS_PREFIX else GIT_TEST_INSTALLED="$mydir/bin-wrappers" # Older versions of git lacked bin-wrappers; fallback to the # files in the root. test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir export GIT_TEST_INSTALLED + export PERF_RESULTS_PREFIX fi run_one_dir "$@" } It does mean that anybody calling "GIT_TEST_INSTALLED=whatever ./p1234-foo.sh" has to pass in PERF_RESULTS_PREFIX, but I think that's OK (they should be using the "run" script anyway). > As seen in 2/2 modifying some of the shell & associated Perl code it's > a bit nasty that we need to duplicate the logic for picking these > PERF_RESULTS_PREFIXes in the shell code and Perl code. Ideally the > shell code would pick it, and pass the mapping to the Perl code > somehow. But that's another "has sucked since forever, future TODO" > item. Yeah, I didn't realize that aggregate.perl has the duplicate logic. Gross. But I agree that's not a topic for now. It would continue to compute the same PERF_RESULTS_PREFIX that "run" does, except it doesn't need to bother to pass it to anybody else. (Note that in my patch above, I dropped the pointless "build_" from the rev-based names, so aggregate.perl would need to do the same). -Peff ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 0/6] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason 2019-05-07 7:06 ` Jeff King @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 21:36 ` Jeff King 2019-05-07 10:54 ` [PATCH v3 1/6] perf README: correct docs for 3c8f12c96c regression Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 7 siblings, 1 reply; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason So here as a 6-parter given some of the feedback on v2. Maybe Jeff still hates it :), but this time around some of the changes are split up and should be easier to understand in isolation, as well as some more "noticed while I was at it" things fixed. This series ends with outright forbidding the user from directly setting GIT_TEST_INSTALLED. As discussed it makes things easier for us, and as noted in 1/6 the demand for that in the wild seems non-existent, since the way we've been documenting how you could do that with an environment variable has been broken since 2012. Ævar Arnfjörð Bjarmason (6): perf README: correct docs for 3c8f12c96c regression perf aggregate: remove GIT_TEST_INSTALLED from --codespeed perf-lib.sh: make "./run <revisions>" use the correct gits perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh perf tests: add "bindir" prefix to git tree test results perf-lib.sh: forbid the use of GIT_TEST_INSTALLED t/perf/README | 2 +- t/perf/aggregate.perl | 17 +++++++++-------- t/perf/perf-lib.sh | 18 ++++++++++-------- t/perf/run | 43 +++++++++++++++++++++++++++++++++---------- 4 files changed, 53 insertions(+), 27 deletions(-) Range-diff: -: ---------- > 1: 6684dca042 perf README: correct docs for 3c8f12c96c regression -: ---------- > 2: c4e903d898 perf aggregate: remove GIT_TEST_INSTALLED from --codespeed 1: 22a132ed64 ! 3: 9d2d162c64 perf-lib.sh: make "./run <revisions>" use the correct gits @@ -40,9 +40,13 @@ this will subtly fail in test-lib.sh. This has always been the case even before 0baf78e7bc, and as documented in t/README the GIT_TEST_INSTALLED variable should be set to an absolute path (needs - to be set "to the bindir", which is always absolute). Perhaps that - should be dealt with in the future, but I'm leaving that alone for - now. + to be set "to the bindir", which is always absolute), and the "perf" + framework expects to munge it itself. + + Perhaps that should be dealt with in the future to allow manually + setting GIT_TEST_INSTALLED, but as a preceding commit showed the user + can just use the "run" script, which'll also pick the right output + directory for the test results as expected by aggregate.perl. 1. https://public-inbox.org/git/20190502222409.GA15631@sigill.intra.peff.net/ 2: f43beb6450 ! 4: 58f1dd3f6f perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh @@ -26,28 +26,28 @@ We'll just do the right thing because PERF_RESULTS_PREFIX will be empty, and test-lib.sh takes care of finding where our git is. - Refactoring this revealed a few bugs, e.g. while a relative git path - was supported via e.g.: + Any refactoring of this code needs to change both the shell code and + the Perl code in aggregate.perl, because when running e.g.: ./run ../../ -- <test> - We'd just print out ".." as the header, since we'd always take the - content after the last slash. Now we'll always resolve the absolute - path to something we detect to be be a manually supplied bindir, and - print the full path in the aggregation. + The "../../" path to a relative bindir needs to be munged to a + filename containing the results, and critically aggregate.perl does + not get passed the path to those aggregations, just "../..". - There was also a long-standing bug in the codespeed output where the - "environment" for N number of tests would be whatever our - GIT_TEST_INSTALLED had been set to by the last of those N runs. Let's - instead just fall back to "uname -r", which is a more sensible - "environment" than some random build directory path, even for the N=1 - case. + Let's fix cases where aggregate.perl would print e.g. ".." in its + report output for this, and "git" for "/home/avar/g/git", i.e. it + would always pick the last element. Now'll always print the full path + instead. + + This also makes the code sturdier, e.g. you can feed "../.." to + "./run" and then an absolute path to the aggregate.perl script, as + long as the absolute path and "../.." resolved to the same directory + printing the aggregation will work. Also simplify the "[_*]" on the RHS of "tr -c", we're trimming everything to "_", so we don't need that. - https://public-inbox.org/git/20190502222409.GA15631@sigill.intra.peff.net/ - Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl @@ -80,16 +80,6 @@ my $prefix = $dir; $prefix =~ tr/^a-zA-Z0-9/_/c; $prefixes{$dir} = $prefix . '.'; -@@ - $environment = $reponame; - } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") { - $environment = $ENV{GIT_PERF_REPO_NAME}; -- } elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") { -- $environment = $ENV{GIT_TEST_INSTALLED}; -- $environment =~ s|/bin-wrappers$||; - } else { - $environment = `uname -r`; - chomp $environment; diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh --- a/t/perf/perf-lib.sh -: ---------- > 5: 64df9157a4 perf tests: add "bindir" prefix to git tree test results -: ---------- > 6: 21307f1f2d perf-lib.sh: forbid the use of GIT_TEST_INSTALLED -- 2.21.0.593.g511ec345e18 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/6] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs 2019-05-07 10:54 ` [PATCH v3 0/6] " Ævar Arnfjörð Bjarmason @ 2019-05-07 21:36 ` Jeff King 2019-05-08 1:59 ` Junio C Hamano 0 siblings, 1 reply; 38+ messages in thread From: Jeff King @ 2019-05-07 21:36 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Christian Couder On Tue, May 07, 2019 at 12:54:28PM +0200, Ævar Arnfjörð Bjarmason wrote: > So here as a 6-parter given some of the feedback on v2. Maybe Jeff > still hates it :), but this time around some of the changes are split > up and should be easier to understand in isolation, as well as some > more "noticed while I was at it" things fixed. Thanks, the split did make things easier to understand. These all look reasonable to me, and I confirmed by re-running a few of the problematic perf tests that everything is behaving as expected after the patches. > This series ends with outright forbidding the user from directly > setting GIT_TEST_INSTALLED. As discussed it makes things easier for > us, and as noted in 1/6 the demand for that in the wild seems > non-existent, since the way we've been documenting how you could do > that with an environment variable has been broken since 2012. I think this is reasonable, especially with the feature in patch 6 to tell the caller they're doing it wrong, instead of just silently producing nonsense results. Junio, if you haven't been following closely, this can replace my patch from jk/perf-installed-fix. -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 0/6] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs 2019-05-07 21:36 ` Jeff King @ 2019-05-08 1:59 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2019-05-08 1:59 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Christian Couder Jeff King <peff@peff.net> writes: > I think this is reasonable, especially with the feature in patch 6 to > tell the caller they're doing it wrong, instead of just silently > producing nonsense results. > > Junio, if you haven't been following closely, this can replace my patch > from jk/perf-installed-fix. I did not pick up v2 or earlier as it wasn't clear what the resolution would be, but I agree that this round looks sensible. Thanks for the hint; very very much appreciated. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v3 1/6] perf README: correct docs for 3c8f12c96c regression 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason 2019-05-07 7:06 ` Jeff King 2019-05-07 10:54 ` [PATCH v3 0/6] " Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 2/6] perf aggregate: remove GIT_TEST_INSTALLED from --codespeed Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Since 3c8f12c96c ("test-lib: reorder and include GIT-BUILD-OPTIONS a lot earlier", 2012-06-24) the suggested advice of overriding GIT_BUILD_DIR has not worked. We've printed a hard error like this given e.g. GIT_BUILD_DIR=/home/avar/g/git: /bin-wrappers/git is not executable; using GIT_EXEC_PATH error: You haven't built things yet, have you? Let's just suggest that the user run other gits via the "run" script. That'll do the right thing for setting the path to the other git, and running the "aggregate.perl" scripts afterwards will work. As an aside, if setting GIT_BUILD_DIR had still worked, then the MODERN_GIT feature/fix added in 1a0962dee5 ("t/perf: fix regression in testing older versions of git", 2016-06-22) would have broke. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/README | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/perf/README b/t/perf/README index be12090c38..c7b70e2d28 100644 --- a/t/perf/README +++ b/t/perf/README @@ -45,7 +45,7 @@ call the aggregation script to summarize the results: $ ./p0001-rev-list.sh [...] - $ GIT_BUILD_DIR=/path/to/other/git ./p0001-rev-list.sh + $ ./run /path/to/other/git -- ./p0001-rev-list.sh [...] $ ./aggregate.perl . /path/to/other/git ./p0001-rev-list.sh -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 2/6] perf aggregate: remove GIT_TEST_INSTALLED from --codespeed 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2019-05-07 10:54 ` [PATCH v3 1/6] perf README: correct docs for 3c8f12c96c regression Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 3/6] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Remove the setting of the "environment" from the --codespeed output. I don't think this is useful, and it helps with a later refactoring where we GIT_TEST_INSTALLED stop munging/reading GIT_TEST_INSTALLED in the perf tests in so many places. This was added in 05eb1c37ed ("perf/aggregate: implement codespeed JSON output", 2018-01-05), but since the "run" scripts uses "GIT_TEST_INSTALLED" internally this was only ever useful for one-off runs of a single revision as all the "environment" values would be ones for whatever directory the "run" script ran last. Let's instead fall back on the "uname -r" case, which is the sort of thing the environment should be set to, not something that duplicates other parts of the codpseed output. For setting the "environment" to something custom the perf.repoName variable can be used. See 19cf57a92e ("perf/run: read GIT_PERF_REPO_NAME from perf.repoName", 2018-01-05). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/aggregate.perl | 3 --- 1 file changed, 3 deletions(-) diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl index 494907a892..f6518339dc 100755 --- a/t/perf/aggregate.perl +++ b/t/perf/aggregate.perl @@ -312,9 +312,6 @@ sub print_codespeed_results { $environment = $reponame; } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") { $environment = $ENV{GIT_PERF_REPO_NAME}; - } elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") { - $environment = $ENV{GIT_TEST_INSTALLED}; - $environment =~ s|/bin-wrappers$||; } else { $environment = `uname -r`; chomp $environment; -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 3/6] perf-lib.sh: make "./run <revisions>" use the correct gits 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2019-05-07 10:54 ` [PATCH v3 2/6] perf aggregate: remove GIT_TEST_INSTALLED from --codespeed Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 4/6] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Fix a really bad regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15). Since that change all runs of different <revisions> of git have used the git found in the user's $PATH, e.g. /usr/bin/git instead of the <revision> we just built and wanted to performance test. The problem starts with GIT_TEST_INSTALLED not working like our non-perf tests with the "run" script. I.e. you can't run performance tests against a given installed git. Instead we expect to use it ourselves to point GIT_TEST_INSTALLED to the <revision> we just built. However, we had been relying on '$(cd "$GIT_TEST_INSTALLED" && pwd)' to resolve that relative $GIT_TEST_INSTALLED to an absolute path *before* test-lib.sh was loaded, in cases where it was e.g. "build/<rev>/bin-wrappers" and we wanted "<abs_path>build/...". This change post-dates another proposed solution by a few days[1], I didn't notice that version when I initially wrote this. I'm doing the most minimal thing to solve the regression here, a follow-up change will move this result prefix selection logic entirely into the "run" script. This makes e.g. these cases all work: ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> As well as just a plain one-off: ./run <tests> And, since we're passing down the new GIT_PERF_DIR_MYDIR_REL we make sure the bug relating to aggregate.perl not finding our files as described in 0baf78e7bc doesn't happen again. What *doesn't* work is setting GIT_TEST_INSTALLED to a relative path, this will subtly fail in test-lib.sh. This has always been the case even before 0baf78e7bc, and as documented in t/README the GIT_TEST_INSTALLED variable should be set to an absolute path (needs to be set "to the bindir", which is always absolute), and the "perf" framework expects to munge it itself. Perhaps that should be dealt with in the future to allow manually setting GIT_TEST_INSTALLED, but as a preceding commit showed the user can just use the "run" script, which'll also pick the right output directory for the test results as expected by aggregate.perl. 1. https://public-inbox.org/git/20190502222409.GA15631@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/perf-lib.sh | 4 ++++ t/perf/run | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..b15ee1d262 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t if test -z "$GIT_TEST_INSTALLED"; then perf_results_prefix= else + if test -n "$GIT_PERF_DIR_MYDIR_REL" + then + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL + fi perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED fi diff --git a/t/perf/run b/t/perf/run index 9aaa733c77..0a7c8744ab 100755 --- a/t/perf/run +++ b/t/perf/run @@ -91,10 +91,14 @@ run_dirs_helper () { if test "$mydir" = .; then unset GIT_TEST_INSTALLED else - GIT_TEST_INSTALLED="$mydir/bin-wrappers" + GIT_PERF_DIR_MYDIR_REL=$mydir + GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) + export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS + + GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" # Older versions of git lacked bin-wrappers; fallback to the # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir + test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS export GIT_TEST_INSTALLED fi run_one_dir "$@" -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 4/6] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2019-05-07 10:54 ` [PATCH v3 3/6] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 5/6] perf tests: add "bindir" prefix to git tree test results Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 6/6] perf-lib.sh: forbid the use of GIT_TEST_INSTALLED Ævar Arnfjörð Bjarmason 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Follow-up my preceding change which fixed the immediate "./run <revisions>" regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15) and entirely get rid of GIT_TEST_INSTALLED from perf-lib.sh (and aggregate.perl). As noted in that change the dance we're doing with GIT_TEST_INSTALLED perf-lib.sh isn't necessary, but there I was doing the most minimal set of changes to quickly fix a regression. But it's much simpler to never deal with the "GIT_TEST_INSTALLED" we were setting in perf-lib.sh at all. Instead the run_dirs_helper() sets the previously inferred $PERF_RESULTS_PREFIX directly. Setting this at the callsite that's already best positioned to exhaustively know about all the different cases we need to handle where PERF_RESULTS_PREFIX isn't what we want already (the empty string) makes the most sense. In one-off cases like: ./run ./p0000-perf-lib-sanity.sh ./p0000-perf-lib-sanity.sh We'll just do the right thing because PERF_RESULTS_PREFIX will be empty, and test-lib.sh takes care of finding where our git is. Any refactoring of this code needs to change both the shell code and the Perl code in aggregate.perl, because when running e.g.: ./run ../../ -- <test> The "../../" path to a relative bindir needs to be munged to a filename containing the results, and critically aggregate.perl does not get passed the path to those aggregations, just "../..". Let's fix cases where aggregate.perl would print e.g. ".." in its report output for this, and "git" for "/home/avar/g/git", i.e. it would always pick the last element. Now'll always print the full path instead. This also makes the code sturdier, e.g. you can feed "../.." to "./run" and then an absolute path to the aggregate.perl script, as long as the absolute path and "../.." resolved to the same directory printing the aggregation will work. Also simplify the "[_*]" on the RHS of "tr -c", we're trimming everything to "_", so we don't need that. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/aggregate.perl | 10 ++++++---- t/perf/perf-lib.sh | 15 +-------------- t/perf/run | 45 +++++++++++++++++++++++++++++-------------- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl index f6518339dc..c8f4a78903 100755 --- a/t/perf/aggregate.perl +++ b/t/perf/aggregate.perl @@ -6,6 +6,7 @@ use JSON; use Getopt::Long; use Git; +use Cwd qw(realpath); sub get_times { my $name = shift; @@ -103,13 +104,14 @@ sub format_size { if (! -d $arg) { my $rev = Git::command_oneline(qw(rev-parse --verify), $arg); $dir = "build/".$rev; + } elsif ($arg eq '.') { + $dir = '.'; } else { - $arg =~ s{/*$}{}; - $dir = $arg; - $dirabbrevs{$dir} = $dir; + $dir = realpath($arg); + $dirnames{$dir} = $dir; } push @dirs, $dir; - $dirnames{$dir} = $arg; + $dirnames{$dir} ||= $arg; my $prefix = $dir; $prefix =~ tr/^a-zA-Z0-9/_/c; $prefixes{$dir} = $prefix . '.'; diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index b15ee1d262..9cdccba222 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -21,25 +21,12 @@ # because it will change our working directory. TEST_DIRECTORY=$(pwd)/.. TEST_OUTPUT_DIRECTORY=$(pwd) -ABSOLUTE_GIT_TEST_INSTALLED=$( - test -n "$GIT_TEST_INSTALLED" && cd "$GIT_TEST_INSTALLED" && pwd) TEST_NO_CREATE_REPO=t TEST_NO_MALLOC_CHECK=t . ../test-lib.sh -if test -z "$GIT_TEST_INSTALLED"; then - perf_results_prefix= -else - if test -n "$GIT_PERF_DIR_MYDIR_REL" - then - GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL - fi - perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." - GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED -fi - # Variables from test-lib that are normally internal to the tests; we # need to export them for test_perf subshells export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP @@ -183,7 +170,7 @@ test_wrapper_ () { base=$(basename "$0" .sh) echo "$test_count" >>"$perf_results_dir"/$base.subtests echo "$1" >"$perf_results_dir"/$base.$test_count.descr - base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count" + base="$perf_results_dir"/"$PERF_RESULTS_PREFIX$(basename "$0" .sh)"."$test_count" "$test_wrapper_func_" "$@" fi diff --git a/t/perf/run b/t/perf/run index 0a7c8744ab..85b7bd31d5 100755 --- a/t/perf/run +++ b/t/perf/run @@ -70,6 +70,22 @@ build_git_rev () { ) || die "failed to build revision '$mydir'" } +set_git_test_installed () { + mydir=$1 + + mydir_abs=$(cd $mydir && pwd) + mydir_abs_wrappers="$mydir_abs_wrappers/bin-wrappers" + if test -d "$mydir_abs_wrappers" + then + GIT_TEST_INSTALLED=$mydir_abs_wrappers + else + # Older versions of git lacked bin-wrappers; + # fallback to the files in the root. + GIT_TEST_INSTALLED=$mydir_abs + fi + export GIT_TEST_INSTALLED +} + run_dirs_helper () { mydir=${1%/} shift @@ -79,7 +95,16 @@ run_dirs_helper () { if test $# -gt 0 -a "$1" = --; then shift fi - if [ ! -d "$mydir" ]; then + + PERF_RESULTS_PREFIX= + if test "$mydir" = "." + then + unset GIT_TEST_INSTALLED + elif test -d "$mydir" + then + PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). + set_git_test_installed "$mydir" + else rev=$(git rev-parse --verify "$mydir" 2>/dev/null) || die "'$mydir' is neither a directory nor a valid revision" if [ ! -d build/$rev ]; then @@ -87,20 +112,12 @@ run_dirs_helper () { fi build_git_rev $rev "$mydir" mydir=build/$rev + + PERF_RESULTS_PREFIX=build_$rev. + set_git_test_installed "$mydir" fi - if test "$mydir" = .; then - unset GIT_TEST_INSTALLED - else - GIT_PERF_DIR_MYDIR_REL=$mydir - GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) - export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS - - GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" - # Older versions of git lacked bin-wrappers; fallback to the - # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS - export GIT_TEST_INSTALLED - fi + export PERF_RESULTS_PREFIX + run_one_dir "$@" } -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 5/6] perf tests: add "bindir" prefix to git tree test results 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2019-05-07 10:54 ` [PATCH v3 4/6] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 6/6] perf-lib.sh: forbid the use of GIT_TEST_INSTALLED Ævar Arnfjörð Bjarmason 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Change the output file names in test-results/ to be "test-results/bindir_<munged dir>" rather than just "test-results/<munged dir>". This is for consistency with the "build_" directories we have for built revisions, i.e. "test-results/build_<SHA-1>". There's no user-visible functional changes here, it just makes it easier to see at a glance what "test-results" files are of what "type" as they're all explicitly grouped together now, and to grep this code to find both the run_dirs_helper() implementation and its corresponding aggregate.perl code. Note that we already guarantee that the rest of the PERF_RESULTS_PREFIX is an absolute path, and since it'll start with e.g. "/" which we munge to "_" we'll up with a readable string like "bindir_home_avar[...]". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/aggregate.perl | 4 +++- t/perf/run | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl index c8f4a78903..b951747e08 100755 --- a/t/perf/aggregate.perl +++ b/t/perf/aggregate.perl @@ -100,6 +100,7 @@ sub format_size { while (scalar @ARGV) { my $arg = $ARGV[0]; my $dir; + my $prefix = ''; last if -f $arg or $arg eq "--"; if (! -d $arg) { my $rev = Git::command_oneline(qw(rev-parse --verify), $arg); @@ -109,10 +110,11 @@ sub format_size { } else { $dir = realpath($arg); $dirnames{$dir} = $dir; + $prefix .= 'bindir'; } push @dirs, $dir; $dirnames{$dir} ||= $arg; - my $prefix = $dir; + $prefix .= $dir; $prefix =~ tr/^a-zA-Z0-9/_/c; $prefixes{$dir} = $prefix . '.'; shift @ARGV; diff --git a/t/perf/run b/t/perf/run index 85b7bd31d5..cd3882b117 100755 --- a/t/perf/run +++ b/t/perf/run @@ -102,7 +102,7 @@ run_dirs_helper () { unset GIT_TEST_INSTALLED elif test -d "$mydir" then - PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). + PERF_RESULTS_PREFIX=bindir$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). set_git_test_installed "$mydir" else rev=$(git rev-parse --verify "$mydir" 2>/dev/null) || -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v3 6/6] perf-lib.sh: forbid the use of GIT_TEST_INSTALLED 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason ` (6 preceding siblings ...) 2019-05-07 10:54 ` [PATCH v3 5/6] perf tests: add "bindir" prefix to git tree test results Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 ` Ævar Arnfjörð Bjarmason 7 siblings, 0 replies; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 10:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason As noted in preceding commits setting GIT_TEST_INSTALLED has never been supported or documented, and as noted in an earlier t/perf/README change to the extent that it's been documented nobody's notices that the example hasn't worked since 3c8f12c96c ("test-lib: reorder and include GIT-BUILD-OPTIONS a lot earlier", 2012-06-24). We could directly support GIT_TEST_INSTALLED for invocations without the "run" script, such as: GIT_TEST_INSTALLED=../../ ./p0000-perf-lib-sanity.sh GIT_TEST_INSTALLED=/home/avar/g/git ./p0000-perf-lib-sanity.sh But while not having this "error" will "work", it won't write the the resulting "test-results/*" files to the right place, and thus a subsequent call to aggregate.perl won't work as expected. Let's just tell the user that they need to use the "run" script, which'll correctly deal with this and set the right PERF_RESULTS_PREFIX. If someone's in desperate need of bypassing "run" for whatever reason they can trivially do so by setting "PERF_SET_GIT_TEST_INSTALLED", but not we won't have people who expect GIT_TEST_INSTALLED to just work wondering why their aggregation doesn't work, even though they're running the right "git". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/perf-lib.sh | 11 +++++++++++ t/perf/run | 2 ++ 2 files changed, 13 insertions(+) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 9cdccba222..b58a43ea43 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -27,6 +27,17 @@ TEST_NO_MALLOC_CHECK=t . ../test-lib.sh +if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED" +then + error "Do not use GIT_TEST_INSTALLED with the perf tests. + +Instead use: + + ./run <path-to-git> -- <tests> + +See t/perf/README for details." +fi + # Variables from test-lib that are normally internal to the tests; we # need to export them for test_perf subshells export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP diff --git a/t/perf/run b/t/perf/run index cd3882b117..c7b86104e1 100755 --- a/t/perf/run +++ b/t/perf/run @@ -84,6 +84,8 @@ set_git_test_installed () { GIT_TEST_INSTALLED=$mydir_abs fi export GIT_TEST_INSTALLED + PERF_SET_GIT_TEST_INSTALLED=true + export PERF_SET_GIT_TEST_INSTALLED } run_dirs_helper () { -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits 2019-05-06 20:24 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason @ 2019-05-06 23:23 ` Ævar Arnfjörð Bjarmason 2019-05-07 7:19 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-06 23:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Fix a really bad regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15). Since that change all runs of different <revisions> of git have used the git found in the user's $PATH, e.g. /usr/bin/git instead of the <revision> we just built and wanted to performance test. The problem starts with GIT_TEST_INSTALLED not working like our non-perf tests with the "run" script. I.e. you can't run performance tests against a given installed git. Instead we expect to use it ourselves to point GIT_TEST_INSTALLED to the <revision> we just built. However, we had been relying on '$(cd "$GIT_TEST_INSTALLED" && pwd)' to resolve that relative $GIT_TEST_INSTALLED to an absolute path *before* test-lib.sh was loaded, in cases where it was e.g. "build/<rev>/bin-wrappers" and we wanted "<abs_path>build/...". This change post-dates another proposed solution by a few days[1], I didn't notice that version when I initially wrote this. I'm doing the most minimal thing to solve the regression here, a follow-up change will move this result prefix selection logic entirely into the "run" script. This makes e.g. these cases all work: ./run . $PWD/../../ origin/master origin/next HEAD -- <tests> As well as just a plain one-off: ./run <tests> And, since we're passing down the new GIT_PERF_DIR_MYDIR_REL we make sure the bug relating to aggregate.perl not finding our files as described in 0baf78e7bc doesn't happen again. What *doesn't* work is setting GIT_TEST_INSTALLED to a relative path, this will subtly fail in test-lib.sh. This has always been the case even before 0baf78e7bc, and as documented in t/README the GIT_TEST_INSTALLED variable should be set to an absolute path (needs to be set "to the bindir", which is always absolute). Perhaps that should be dealt with in the future, but I'm leaving that alone for now. 1. https://public-inbox.org/git/20190502222409.GA15631@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/perf-lib.sh | 4 ++++ t/perf/run | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 169f92eae3..b15ee1d262 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t if test -z "$GIT_TEST_INSTALLED"; then perf_results_prefix= else + if test -n "$GIT_PERF_DIR_MYDIR_REL" + then + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL + fi perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED fi diff --git a/t/perf/run b/t/perf/run index 9aaa733c77..0a7c8744ab 100755 --- a/t/perf/run +++ b/t/perf/run @@ -91,10 +91,14 @@ run_dirs_helper () { if test "$mydir" = .; then unset GIT_TEST_INSTALLED else - GIT_TEST_INSTALLED="$mydir/bin-wrappers" + GIT_PERF_DIR_MYDIR_REL=$mydir + GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) + export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS + + GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" # Older versions of git lacked bin-wrappers; fallback to the # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$mydir + test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS export GIT_TEST_INSTALLED fi run_one_dir "$@" -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits 2019-05-06 23:23 ` [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason @ 2019-05-07 7:19 ` Jeff King 0 siblings, 0 replies; 38+ messages in thread From: Jeff King @ 2019-05-07 7:19 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Christian Couder On Tue, May 07, 2019 at 01:23:08AM +0200, Ævar Arnfjörð Bjarmason wrote: > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh > index 169f92eae3..b15ee1d262 100644 > --- a/t/perf/perf-lib.sh > +++ b/t/perf/perf-lib.sh > @@ -32,6 +32,10 @@ TEST_NO_MALLOC_CHECK=t > if test -z "$GIT_TEST_INSTALLED"; then > perf_results_prefix= > else > + if test -n "$GIT_PERF_DIR_MYDIR_REL" > + then > + GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL > + fi > perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." > GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED > fi I still like my fix better, as it works completely within perf-lib.sh, and so is a more direct fix. And doesn't have this weird exported MYDIR_ABS that nobody actually uses. But I don't actually think this topic is worth spending too many brain cycles on. So I am fine with either fix, as long as we do something. -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh 2019-05-06 20:24 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason 2019-05-06 23:23 ` [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason @ 2019-05-06 23:23 ` Ævar Arnfjörð Bjarmason 2019-05-07 7:16 ` Jeff King 2 siblings, 1 reply; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-06 23:23 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Christian Couder, Ævar Arnfjörð Bjarmason Follow-up my preceding change which fixed the immediate "./run <revisions>" regression in 0baf78e7bc ("perf-lib.sh: rely on test-lib.sh for --tee handling", 2019-03-15) and entirely get rid of GIT_TEST_INSTALLED from perf-lib.sh (and aggregate.perl). As noted in that change the dance we're doing with GIT_TEST_INSTALLED perf-lib.sh isn't necessary, but there I was doing the most minimal set of changes to quickly fix a regression. But it's much simpler to never deal with the "GIT_TEST_INSTALLED" we were setting in perf-lib.sh at all. Instead the run_dirs_helper() sets the previously inferred $PERF_RESULTS_PREFIX directly. Setting this at the callsite that's already best positioned to exhaustively know about all the different cases we need to handle where PERF_RESULTS_PREFIX isn't what we want already (the empty string) makes the most sense. In one-off cases like: ./run ./p0000-perf-lib-sanity.sh ./p0000-perf-lib-sanity.sh We'll just do the right thing because PERF_RESULTS_PREFIX will be empty, and test-lib.sh takes care of finding where our git is. Refactoring this revealed a few bugs, e.g. while a relative git path was supported via e.g.: ./run ../../ -- <test> We'd just print out ".." as the header, since we'd always take the content after the last slash. Now we'll always resolve the absolute path to something we detect to be be a manually supplied bindir, and print the full path in the aggregation. There was also a long-standing bug in the codespeed output where the "environment" for N number of tests would be whatever our GIT_TEST_INSTALLED had been set to by the last of those N runs. Let's instead just fall back to "uname -r", which is a more sensible "environment" than some random build directory path, even for the N=1 case. Also simplify the "[_*]" on the RHS of "tr -c", we're trimming everything to "_", so we don't need that. https://public-inbox.org/git/20190502222409.GA15631@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/perf/aggregate.perl | 13 ++++++------- t/perf/perf-lib.sh | 15 +-------------- t/perf/run | 45 +++++++++++++++++++++++++++++-------------- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl index 494907a892..c8f4a78903 100755 --- a/t/perf/aggregate.perl +++ b/t/perf/aggregate.perl @@ -6,6 +6,7 @@ use JSON; use Getopt::Long; use Git; +use Cwd qw(realpath); sub get_times { my $name = shift; @@ -103,13 +104,14 @@ sub format_size { if (! -d $arg) { my $rev = Git::command_oneline(qw(rev-parse --verify), $arg); $dir = "build/".$rev; + } elsif ($arg eq '.') { + $dir = '.'; } else { - $arg =~ s{/*$}{}; - $dir = $arg; - $dirabbrevs{$dir} = $dir; + $dir = realpath($arg); + $dirnames{$dir} = $dir; } push @dirs, $dir; - $dirnames{$dir} = $arg; + $dirnames{$dir} ||= $arg; my $prefix = $dir; $prefix =~ tr/^a-zA-Z0-9/_/c; $prefixes{$dir} = $prefix . '.'; @@ -312,9 +314,6 @@ sub print_codespeed_results { $environment = $reponame; } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") { $environment = $ENV{GIT_PERF_REPO_NAME}; - } elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") { - $environment = $ENV{GIT_TEST_INSTALLED}; - $environment =~ s|/bin-wrappers$||; } else { $environment = `uname -r`; chomp $environment; diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index b15ee1d262..9cdccba222 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -21,25 +21,12 @@ # because it will change our working directory. TEST_DIRECTORY=$(pwd)/.. TEST_OUTPUT_DIRECTORY=$(pwd) -ABSOLUTE_GIT_TEST_INSTALLED=$( - test -n "$GIT_TEST_INSTALLED" && cd "$GIT_TEST_INSTALLED" && pwd) TEST_NO_CREATE_REPO=t TEST_NO_MALLOC_CHECK=t . ../test-lib.sh -if test -z "$GIT_TEST_INSTALLED"; then - perf_results_prefix= -else - if test -n "$GIT_PERF_DIR_MYDIR_REL" - then - GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_REL - fi - perf_results_prefix=$(printf "%s" "${GIT_TEST_INSTALLED%/bin-wrappers}" | tr -c "[a-zA-Z0-9]" "[_*]")"." - GIT_TEST_INSTALLED=$ABSOLUTE_GIT_TEST_INSTALLED -fi - # Variables from test-lib that are normally internal to the tests; we # need to export them for test_perf subshells export TEST_DIRECTORY TRASH_DIRECTORY GIT_BUILD_DIR GIT_TEST_CMP @@ -183,7 +170,7 @@ test_wrapper_ () { base=$(basename "$0" .sh) echo "$test_count" >>"$perf_results_dir"/$base.subtests echo "$1" >"$perf_results_dir"/$base.$test_count.descr - base="$perf_results_dir"/"$perf_results_prefix$(basename "$0" .sh)"."$test_count" + base="$perf_results_dir"/"$PERF_RESULTS_PREFIX$(basename "$0" .sh)"."$test_count" "$test_wrapper_func_" "$@" fi diff --git a/t/perf/run b/t/perf/run index 0a7c8744ab..85b7bd31d5 100755 --- a/t/perf/run +++ b/t/perf/run @@ -70,6 +70,22 @@ build_git_rev () { ) || die "failed to build revision '$mydir'" } +set_git_test_installed () { + mydir=$1 + + mydir_abs=$(cd $mydir && pwd) + mydir_abs_wrappers="$mydir_abs_wrappers/bin-wrappers" + if test -d "$mydir_abs_wrappers" + then + GIT_TEST_INSTALLED=$mydir_abs_wrappers + else + # Older versions of git lacked bin-wrappers; + # fallback to the files in the root. + GIT_TEST_INSTALLED=$mydir_abs + fi + export GIT_TEST_INSTALLED +} + run_dirs_helper () { mydir=${1%/} shift @@ -79,7 +95,16 @@ run_dirs_helper () { if test $# -gt 0 -a "$1" = --; then shift fi - if [ ! -d "$mydir" ]; then + + PERF_RESULTS_PREFIX= + if test "$mydir" = "." + then + unset GIT_TEST_INSTALLED + elif test -d "$mydir" + then + PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). + set_git_test_installed "$mydir" + else rev=$(git rev-parse --verify "$mydir" 2>/dev/null) || die "'$mydir' is neither a directory nor a valid revision" if [ ! -d build/$rev ]; then @@ -87,20 +112,12 @@ run_dirs_helper () { fi build_git_rev $rev "$mydir" mydir=build/$rev + + PERF_RESULTS_PREFIX=build_$rev. + set_git_test_installed "$mydir" fi - if test "$mydir" = .; then - unset GIT_TEST_INSTALLED - else - GIT_PERF_DIR_MYDIR_REL=$mydir - GIT_PERF_DIR_MYDIR_ABS=$(cd $mydir && pwd) - export GIT_PERF_DIR_MYDIR_REL GIT_PERF_DIR_MYDIR_ABS - - GIT_TEST_INSTALLED="$GIT_PERF_DIR_MYDIR_ABS/bin-wrappers" - # Older versions of git lacked bin-wrappers; fallback to the - # files in the root. - test -d "$GIT_TEST_INSTALLED" || GIT_TEST_INSTALLED=$GIT_PERF_DIR_MYDIR_ABS - export GIT_TEST_INSTALLED - fi + export PERF_RESULTS_PREFIX + run_one_dir "$@" } -- 2.21.0.593.g511ec345e18 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh 2019-05-06 23:23 ` [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason @ 2019-05-07 7:16 ` Jeff King 2019-05-07 8:31 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 38+ messages in thread From: Jeff King @ 2019-05-07 7:16 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Christian Couder On Tue, May 07, 2019 at 01:23:09AM +0200, Ævar Arnfjörð Bjarmason wrote: > @@ -79,7 +95,16 @@ run_dirs_helper () { > if test $# -gt 0 -a "$1" = --; then > shift > fi > - if [ ! -d "$mydir" ]; then > + > + PERF_RESULTS_PREFIX= > + if test "$mydir" = "." > + then > + unset GIT_TEST_INSTALLED > + elif test -d "$mydir" > + then > + PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). > + set_git_test_installed "$mydir" > + else > rev=$(git rev-parse --verify "$mydir" 2>/dev/null) || > die "'$mydir' is neither a directory nor a valid revision" > if [ ! -d build/$rev ]; then OK, so this is basically the same cleanup I came up with. The big difference is that you pushed the shared code into a function, rather than the funky double-conditional in the original. I'm OK with that. > diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl > index 494907a892..c8f4a78903 100755 > --- a/t/perf/aggregate.perl > +++ b/t/perf/aggregate.perl > @@ -6,6 +6,7 @@ > use JSON; > use Getopt::Long; > use Git; > +use Cwd qw(realpath); > > sub get_times { > my $name = shift; > @@ -103,13 +104,14 @@ sub format_size { > if (! -d $arg) { > my $rev = Git::command_oneline(qw(rev-parse --verify), $arg); > $dir = "build/".$rev; > + } elsif ($arg eq '.') { > + $dir = '.'; > } else { > - $arg =~ s{/*$}{}; > - $dir = $arg; > - $dirabbrevs{$dir} = $dir; > + $dir = realpath($arg); > + $dirnames{$dir} = $dir; > } > push @dirs, $dir; > - $dirnames{$dir} = $arg; > + $dirnames{$dir} ||= $arg; I'm not sure I get what's going on here. Why do we need the realpath in aggregate.perl? We'd want to generate the same filename that "run" decided to store things in, which we'd generate from the command-line arguments (either passed on to us by "run", or direct from the user if they're printing a previous run). > @@ -312,9 +314,6 @@ sub print_codespeed_results { > $environment = $reponame; > } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") { > $environment = $ENV{GIT_PERF_REPO_NAME}; > - } elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") { > - $environment = $ENV{GIT_TEST_INSTALLED}; > - $environment =~ s|/bin-wrappers$||; > } else { > $environment = `uname -r`; > chomp $environment; Is this codespeed thing a totally separate bug? Should it go into its own patch? -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh 2019-05-07 7:16 ` Jeff King @ 2019-05-07 8:31 ` Ævar Arnfjörð Bjarmason 2019-05-07 21:23 ` Jeff King 0 siblings, 1 reply; 38+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-05-07 8:31 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Christian Couder On Tue, May 07 2019, Jeff King wrote: > On Tue, May 07, 2019 at 01:23:09AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> @@ -79,7 +95,16 @@ run_dirs_helper () { >> if test $# -gt 0 -a "$1" = --; then >> shift >> fi >> - if [ ! -d "$mydir" ]; then >> + >> + PERF_RESULTS_PREFIX= >> + if test "$mydir" = "." >> + then >> + unset GIT_TEST_INSTALLED >> + elif test -d "$mydir" >> + then >> + PERF_RESULTS_PREFIX=$(cd $mydir && printf "%s" "$(pwd)" | tr -c "[a-zA-Z0-9]" "_"). >> + set_git_test_installed "$mydir" >> + else >> rev=$(git rev-parse --verify "$mydir" 2>/dev/null) || >> die "'$mydir' is neither a directory nor a valid revision" >> if [ ! -d build/$rev ]; then > > OK, so this is basically the same cleanup I came up with. The big > difference is that you pushed the shared code into a function, rather > than the funky double-conditional in the original. I'm OK with that. Yup. It can be done either way, but this way allows for a many/many mapping where we set the GIT_TEST_INSTALLED & PERF_RESULTS_PREFIX depending on the different cases, as opposed to in perf-lib.sh where we'd need to set GIT_TEST_INSTALLED and then infer a PERF_RESULTS_PREFIX (or pass flags marking each case, which would amount to the same thing...). >> diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl >> index 494907a892..c8f4a78903 100755 >> --- a/t/perf/aggregate.perl >> +++ b/t/perf/aggregate.perl >> @@ -6,6 +6,7 @@ >> use JSON; >> use Getopt::Long; >> use Git; >> +use Cwd qw(realpath); >> >> sub get_times { >> my $name = shift; >> @@ -103,13 +104,14 @@ sub format_size { >> if (! -d $arg) { >> my $rev = Git::command_oneline(qw(rev-parse --verify), $arg); >> $dir = "build/".$rev; >> + } elsif ($arg eq '.') { >> + $dir = '.'; >> } else { >> - $arg =~ s{/*$}{}; >> - $dir = $arg; >> - $dirabbrevs{$dir} = $dir; >> + $dir = realpath($arg); >> + $dirnames{$dir} = $dir; >> } >> push @dirs, $dir; >> - $dirnames{$dir} = $arg; >> + $dirnames{$dir} ||= $arg; > > I'm not sure I get what's going on here. Why do we need the realpath in > aggregate.perl? We'd want to generate the same filename that "run" > decided to store things in, which we'd generate from the command-line > arguments (either passed on to us by "run", or direct from the user if > they're printing a previous run). So this is part of the "has sucked since forever, future TODO" mentioned in 0/2. I.e. if you pass "../.." as a path to "run" we'll try to discover a built/installed "git" in a "bindir" there, and then we need to do two things: 1. Figure out a way to turn that into a filename sensible for the *.times files. 2. Print some header showing that path in the aggregate output. The "run" script will discover #1 for itself, that's what that "pwd && tr -c ..." command is doing, but then we just pass "../.." again to aggregate.perl and have it figure it out again on its own, so it needs to duplicate the logic. Just having both discover the absolute path all the time for #1 made things a lot simpler, e.g. if you do ../.. on v2.21.0 you'll get things like: _____.p0000-perf-lib-sanity.1.times And with $PWD/../../ you'd get: _home_avar_g_git_t_perf______.p0000-perf-lib-sanity.1.times Now this is all pretty & consistent. Any path to a "git" will always be turned into the absolute path, e.g.: _home_avar_g_git.p0000-perf-lib-sanity.1.times And instead of "git" or ".." being printed in the aggregate header we print the path, e.g. "/home/avar/g/git". >> @@ -312,9 +314,6 @@ sub print_codespeed_results { >> $environment = $reponame; >> } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") { >> $environment = $ENV{GIT_PERF_REPO_NAME}; >> - } elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} ne "") { >> - $environment = $ENV{GIT_TEST_INSTALLED}; >> - $environment =~ s|/bin-wrappers$||; >> } else { >> $environment = `uname -r`; >> chomp $environment; > > Is this codespeed thing a totally separate bug? Should it go into its > own patch? I could split it up, but figured any change like this would have had to deal with and refactor with any existing uses of GIT_TEST_INSTALLED, that there was a bug in some of that code and it could be removed was just luck... ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh 2019-05-07 8:31 ` Ævar Arnfjörð Bjarmason @ 2019-05-07 21:23 ` Jeff King 0 siblings, 0 replies; 38+ messages in thread From: Jeff King @ 2019-05-07 21:23 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Christian Couder On Tue, May 07, 2019 at 10:31:23AM +0200, Ævar Arnfjörð Bjarmason wrote: > > I'm not sure I get what's going on here. Why do we need the realpath in > > aggregate.perl? We'd want to generate the same filename that "run" > > decided to store things in, which we'd generate from the command-line > > arguments (either passed on to us by "run", or direct from the user if > > they're printing a previous run). > > So this is part of the "has sucked since forever, future TODO" mentioned > in 0/2. > > I.e. if you pass "../.." as a path to "run" we'll try to discover a > built/installed "git" in a "bindir" there, and then we need to do two > things: > > 1. Figure out a way to turn that into a filename sensible for the > *.times files. > 2. Print some header showing that path in the aggregate output. > > The "run" script will discover #1 for itself, that's what that "pwd && > tr -c ..." command is doing, but then we just pass "../.." again to > aggregate.perl and have it figure it out again on its own, so it needs > to duplicate the logic. > > Just having both discover the absolute path all the time for #1 made > things a lot simpler, e.g. if you do ../.. on v2.21.0 you'll get things > like: > > _____.p0000-perf-lib-sanity.1.times > > And with $PWD/../../ you'd get: > > _home_avar_g_git_t_perf______.p0000-perf-lib-sanity.1.times > > Now this is all pretty & consistent. Any path to a "git" will always be > turned into the absolute path, e.g.: > > _home_avar_g_git.p0000-perf-lib-sanity.1.times > > And instead of "git" or ".." being printed in the aggregate header we > print the path, e.g. "/home/avar/g/git". OK. I sort of assumed we'd be sticking with the crappy "_____" for both cases after your cleanup. But it really is changing behavior to name things after the absolute path. I'd probably have split that out into its own change, but I don't think it's worth revisiting at this point. Thanks for explaining. -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano 2019-04-25 14:50 ` Elijah Newren 2019-04-25 22:16 ` js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) Josh Steadmon @ 2019-04-26 5:05 ` Taylor Blau 2019-04-26 5:41 ` Junio C Hamano 2019-04-29 22:20 ` js/macos-gettext-build, was " Johannes Schindelin 2019-05-06 9:11 ` Duy Nguyen 4 siblings, 1 reply; 38+ messages in thread From: Taylor Blau @ 2019-04-26 5:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi Junio, On Thu, Apr 25, 2019 at 07:15:06PM +0900, Junio C Hamano wrote: > * tb/unexpected (2019-04-10) 7 commits > (merged to 'next' on 2019-04-25 at c49927fca0) > + rev-list: detect broken root trees > + rev-list: let traversal die when --missing is not in use > + get_commit_tree(): return NULL for broken tree > + list-objects.c: handle unexpected non-tree entries > + list-objects.c: handle unexpected non-blob entries > + t: introduce tests for unexpected object types > + t: move 'hex2oct' into test-lib-functions.sh > > Code tightening against a "wrong" object appearing where an object > of a different type is expected, instead of blindly assuming that > the connection between objects are correctly made. > > Will merge to 'master'. Thanks for picking this up. Before you merge to master, I want to make sure that the whole series was taken in. I can see the first four of these landed on 'next' (in 5c07647d98...b49e74eac4), but I'm having some difficulty finding the later three. Did you pick these up as well? Thanks, Taylor ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-26 5:05 ` What's cooking in git.git (Apr 2019, #05; Thu, 25) Taylor Blau @ 2019-04-26 5:41 ` Junio C Hamano 2019-04-26 5:53 ` Taylor Blau 2019-04-26 17:58 ` Kaartic Sivaraam 0 siblings, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2019-04-26 5:41 UTC (permalink / raw) To: Taylor Blau; +Cc: git Taylor Blau <me@ttaylorr.com> writes: > On Thu, Apr 25, 2019 at 07:15:06PM +0900, Junio C Hamano wrote: >> * tb/unexpected (2019-04-10) 7 commits >> (merged to 'next' on 2019-04-25 at c49927fca0) >> + rev-list: detect broken root trees >> + rev-list: let traversal die when --missing is not in use >> + get_commit_tree(): return NULL for broken tree >> + list-objects.c: handle unexpected non-tree entries >> + list-objects.c: handle unexpected non-blob entries >> + t: introduce tests for unexpected object types >> + t: move 'hex2oct' into test-lib-functions.sh >> >> Code tightening against a "wrong" object appearing where an object >> of a different type is expected, instead of blindly assuming that >> the connection between objects are correctly made. >> >> Will merge to 'master'. > > Thanks for picking this up. Before you merge to master, I want to make > sure that the whole series was taken in. > > I can see the first four of these landed on 'next' (in > 5c07647d98...b49e74eac4), but I'm having some difficulty finding the > later three. > > Did you pick these up as well? Sorry, but I do not follow. $ git log --oneline master..c49927fca0^2 97dd512af7 rev-list: detect broken root trees ee4dfee227 rev-list: let traversal die when --missing is not in use 834876630b get_commit_tree(): return NULL for broken tree b49e74eac4 list-objects.c: handle unexpected non-tree entries 23c204455b list-objects.c: handle unexpected non-blob entries 0616617c7e t: introduce tests for unexpected object types 5c07647d98 t: move 'hex2oct' into test-lib-functions.sh Do you mean you do not have b49e74eac4..c49927fca0? I do not think the topic was merged in multiple steps (iow, at c49927fca0^ the whole 7 commits were not in 'next', and after c49927fca0, all 7 are in). So I am not sure what you are asking. IOW, if you have b49e74 in your copy of 'next', it is impossible not to have the others. The merge itself can be seen at one of the authoritative repositories https://git.kernel.org/pub/scm/git/git.git/commit/?h=next&id=c49927fca0de4c213ae9b21dcb7eafb80e453d27 Unfortunately, I do not know how to ask GitHub web UI to give us a simple "log --oneline" equivalent of list like gitweb did (sadly cgit is not much better wrt this), but I think clicking on the parent link starting from here https://github.com/git/git/commit/c49927fca0de4c213ae9b21dcb7eafb80e453d27 and remembering (or writing down X-<) the commit names would eventually give us the equivalent. In any case, thanks for working on this topic. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-26 5:41 ` Junio C Hamano @ 2019-04-26 5:53 ` Taylor Blau 2019-04-26 6:01 ` Junio C Hamano 2019-04-26 17:58 ` Kaartic Sivaraam 1 sibling, 1 reply; 38+ messages in thread From: Taylor Blau @ 2019-04-26 5:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git On Fri, Apr 26, 2019 at 02:41:38PM +0900, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > On Thu, Apr 25, 2019 at 07:15:06PM +0900, Junio C Hamano wrote: > >> * tb/unexpected (2019-04-10) 7 commits > >> (merged to 'next' on 2019-04-25 at c49927fca0) > >> + rev-list: detect broken root trees > >> + rev-list: let traversal die when --missing is not in use > >> + get_commit_tree(): return NULL for broken tree > >> + list-objects.c: handle unexpected non-tree entries > >> + list-objects.c: handle unexpected non-blob entries > >> + t: introduce tests for unexpected object types > >> + t: move 'hex2oct' into test-lib-functions.sh > >> > >> Code tightening against a "wrong" object appearing where an object > >> of a different type is expected, instead of blindly assuming that > >> the connection between objects are correctly made. > >> > >> Will merge to 'master'. > > > > Thanks for picking this up. Before you merge to master, I want to make > > sure that the whole series was taken in. > > > > I can see the first four of these landed on 'next' (in > > 5c07647d98...b49e74eac4), but I'm having some difficulty finding the > > later three. > > > > Did you pick these up as well? > > Sorry, but I do not follow. > > $ git log --oneline master..c49927fca0^2 > 97dd512af7 rev-list: detect broken root trees > ee4dfee227 rev-list: let traversal die when --missing is not in use > 834876630b get_commit_tree(): return NULL for broken tree > b49e74eac4 list-objects.c: handle unexpected non-tree entries > 23c204455b list-objects.c: handle unexpected non-blob entries > 0616617c7e t: introduce tests for unexpected object types > 5c07647d98 t: move 'hex2oct' into test-lib-functions.sh > > Do you mean you do not have b49e74eac4..c49927fca0? I do not think > the topic was merged in multiple steps (iow, at c49927fca0^ the > whole 7 commits were not in 'next', and after c49927fca0, all 7 are > in). So I am not sure what you are asking. IOW, if you have b49e74 > in your copy of 'next', it is impossible not to have the others. Ah, I _can_ see the merge in my local copy (fetched from https://github.com/git/git) as c49927fca0 (Merge branch 'tb/unexpected' into next, 2019-04-25). I looked for the commits themselves with: $ git log --author=Taylor on 'next', but only found the first four. I'm sure there's a logical explanation of why this happened, but I'm not sure what it is :-). > The merge itself can be seen at one of the authoritative repositories > > https://git.kernel.org/pub/scm/git/git.git/commit/?h=next&id=c49927fca0de4c213ae9b21dcb7eafb80e453d27 > > Unfortunately, I do not know how to ask GitHub web UI to give us a > simple "log --oneline" equivalent of list like gitweb did (sadly > cgit is not much better wrt this), but I think clicking on the > parent link starting from here > > https://github.com/git/git/commit/c49927fca0de4c213ae9b21dcb7eafb80e453d27 > > and remembering (or writing down X-<) the commit names would > eventually give us the equivalent. > > In any case, thanks for working on this topic. Thanks for explaining, and it was my pleasure. Thanks, Taylor ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-26 5:53 ` Taylor Blau @ 2019-04-26 6:01 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2019-04-26 6:01 UTC (permalink / raw) To: Taylor Blau; +Cc: git Taylor Blau <me@ttaylorr.com> writes: > Ah, I _can_ see the merge in my local copy (fetched from > https://github.com/git/git) as c49927fca0 (Merge branch 'tb/unexpected' > into next, 2019-04-25). > > I looked for the commits themselves with: > > $ git log --author=Taylor > > on 'next', but only found the first four. I'm sure there's a logical > explanation of why this happened, but I'm not sure what it is :-). The last three patches were made out of these messages from you Message-Id: <e0bd479e822ce81de280ee6fdd07f608a96b7836.1554861974.git.me@ttaylorr.com> Message-Id: <88ca5dfe68c4cdb558aff6e90a525ec06f15dbd6.1554861974.git.me@ttaylorr.com> Message-Id: <e9400a9f773adb62315f75347ad7231b4476a2f4.1554861974.git.me@ttaylorr.com> These were you relaying Peff's commits, for example, the topmost one https://public-inbox.org/git/e0bd479e822ce81de280ee6fdd07f608a96b7836.1554861974.git.me@ttaylorr.com/ begins with an in-body header From: with a corresponding sign-off, both by Peff. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-26 5:41 ` Junio C Hamano 2019-04-26 5:53 ` Taylor Blau @ 2019-04-26 17:58 ` Kaartic Sivaraam 2019-04-27 5:06 ` Junio C Hamano 1 sibling, 1 reply; 38+ messages in thread From: Kaartic Sivaraam @ 2019-04-26 17:58 UTC (permalink / raw) To: Junio C Hamano, Taylor Blau; +Cc: git Hi Junio, On 26 April 2019 11:11:38 GMT+05:30, Junio C Hamano <gitster@pobox.com> wrote: > >Unfortunately, I do not know how to ask GitHub web UI to give us a >simple "log --oneline" equivalent of list like gitweb did (sadly >cgit is not much better wrt this), but I think clicking on the >parent link starting from here > >https://github.com/git/git/commit/c49927fca0de4c213ae9b21dcb7eafb80e453d27 > >and remembering (or writing down X-<) the commit names would >eventually give us the equivalent. > May be you are searching for the following view which lists (similar to git log --one-line) the commits starting from 97dd512 which is the last in the tb/unexpected series? https://github.com/git/git/commits/97dd512af7ce4afb4f638ef73b4770921c8ca3aa -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-26 17:58 ` Kaartic Sivaraam @ 2019-04-27 5:06 ` Junio C Hamano 2019-04-27 5:57 ` Kaartic Sivaraam 2019-05-02 3:09 ` Jeff King 0 siblings, 2 replies; 38+ messages in thread From: Junio C Hamano @ 2019-04-27 5:06 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: Taylor Blau, Git Mailing List 2019年4月27日(土) 2:58 Kaartic Sivaraam <kaartic.sivaraam@gmail.com>: > > May be you are searching for the following view which lists (similar to git log --one-line) the commits starting from 97dd512 which is the last in the tb/unexpected series? > > https://github.com/git/git/commits/97dd512af7ce4afb4f638ef73b4770921c8ca3aa Yes, exactly. The problem I have is it is not obvious how to get there starting from https://github.com/git/git/ From that starting point, I can get to https://github.com/git/git/commits/next with two clicks, and get a "git log --oneline next" equivalent). In there, I can eyeball to find "Merge branch 'tb/unexpected' into next". I can click it and reach https://github.com/git/git/commit/c49927fca0d but then that is more like "git show c49927fca". I can even go to its second parent with one click, but that is equivalent to "git show 97dd512". The page you showed, which is a rough equivalent to "git log --oneline 97dd512", is what I want to get to from there, but I am not sure how to get there X-<. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-27 5:06 ` Junio C Hamano @ 2019-04-27 5:57 ` Kaartic Sivaraam 2019-05-02 3:09 ` Jeff King 1 sibling, 0 replies; 38+ messages in thread From: Kaartic Sivaraam @ 2019-04-27 5:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, Git Mailing List Hi Junio, On 27 April 2019 10:36:55 GMT+05:30, Junio C Hamano <gitster@pobox.com> wrote: >2019年4月27日(土) 2:58 Kaartic Sivaraam <kaartic.sivaraam@gmail.com>: >> > >I can click it and reach https://github.com/git/git/commit/c49927fca0d >but then >that is more like "git show c49927fca". I can even go to its second >parent with >one click, but that is equivalent to "git show 97dd512a". The page you >showed, >which is a rough equivalent to "git log --oneline 97dd512", is what I >want to get >to from there, but I am not sure how to get there X-<. I seem to know of two ways to get to where you want to get to (git log --oneline 97dd512) from the page similar to "git show 97dd512" [1]. 1. A direct way that requires two clicks: a. You should click on the 'Browse files' button you see in the page that shows output like git show [1]. Then you'll get to the page where you see the files in the repository just like when you checkout 97dd512 [2]. b. From that page you could click on the number of commits to get to the page you want to see (page which shows output like "git log --oneline 97dd512") [3]. 2. A simple but indirect way is to edit the URL when you are in page [1], and replace the 'commit' part in it with 'commits'. This is not a reliable way as there's a possibility that they could change the URL format but it seems to be working well for me so far :-) Hope that helps. References: [1]: https://github.com/git/git/commit/97dd512af7ce4afb4f638ef73b4770921c8ca3aa [2]: https://github.com/git/git/tree/97dd512af7ce4afb4f638ef73b4770921c8ca3aa [3]: https://github.com/git/git/commits/97dd512af7ce4afb4f638ef73b4770921c8ca3aa -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-27 5:06 ` Junio C Hamano 2019-04-27 5:57 ` Kaartic Sivaraam @ 2019-05-02 3:09 ` Jeff King 1 sibling, 0 replies; 38+ messages in thread From: Jeff King @ 2019-05-02 3:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Kaartic Sivaraam, Taylor Blau, Git Mailing List On Sat, Apr 27, 2019 at 02:06:55PM +0900, Junio C Hamano wrote: > 2019年4月27日(土) 2:58 Kaartic Sivaraam <kaartic.sivaraam@gmail.com>: > > > > May be you are searching for the following view which lists (similar to git log --one-line) the commits starting from 97dd512 which is the last in the tb/unexpected series? > > > > https://github.com/git/git/commits/97dd512af7ce4afb4f638ef73b4770921c8ca3aa > > Yes, exactly. The problem I have is it is not obvious how to get there > starting from > https://github.com/git/git/ Not exactly in git.git, but I'd probably do something like: https://github.com/git/git/compare/master...gitster:tb/unexpected You can get there by going to the "branches" tab in gitster/git, then "tb/unexpected", and then "compare" (either from the branch selection page, or if you've already clicked through from the tree view of that branch). From _just_ inside git.git it's trickier, since it doesn't have that branch. I don't think there's an easy way to click through to a compare-view there, since arbitrary commits that people are looking at might or might not be useful tips for the compare-view (which is also really the pull request view). I'd probably just generate this URL myself: https://github.com/git/git/compare/master...c49927fca0de4c213ae9b21dcb7eafb80e453d27^2 since it's stable and pretty readable. Probably more than you wanted to know, but I feel like I ought to be able to drop at least a little GitHub wisdom. ;) -Peff ^ permalink raw reply [flat|nested] 38+ messages in thread
* js/macos-gettext-build, was Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano ` (2 preceding siblings ...) 2019-04-26 5:05 ` What's cooking in git.git (Apr 2019, #05; Thu, 25) Taylor Blau @ 2019-04-29 22:20 ` Johannes Schindelin 2019-05-05 5:13 ` Junio C Hamano 2019-05-06 9:11 ` Duy Nguyen 4 siblings, 1 reply; 38+ messages in thread From: Johannes Schindelin @ 2019-04-29 22:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Hi Junio, On Thu, 25 Apr 2019, Junio C Hamano wrote: > * js/macos-gettext-build (2019-04-15) 1 commit > (merged to 'next' on 2019-04-22 at de4cbb1431) > + macOS: make sure that gettext is found > > Build with gettext breaks on recent macOS w/ Homebrew when > /usr/local/bin is not on PATH, which has been corrected. This description is almost correct: the problem fixed in this patch is that `/usr/local/include` seems not to be in the header search path of Homebrew's `gcc` on Mojave. The `PATH` does contain `/usr/local/bin` alright. Ciao, Dscho ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: js/macos-gettext-build, was Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-29 22:20 ` js/macos-gettext-build, was " Johannes Schindelin @ 2019-05-05 5:13 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2019-05-05 5:13 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Thu, 25 Apr 2019, Junio C Hamano wrote: > >> * js/macos-gettext-build (2019-04-15) 1 commit >> (merged to 'next' on 2019-04-22 at de4cbb1431) >> + macOS: make sure that gettext is found >> >> Build with gettext breaks on recent macOS w/ Homebrew when >> /usr/local/bin is not on PATH, which has been corrected. > > This description is almost correct: the problem fixed in this patch is > that `/usr/local/include` seems not to be in the header search path of > Homebrew's `gcc` on Mojave. Ack. Thanks. Will update the draft release notes. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano ` (3 preceding siblings ...) 2019-04-29 22:20 ` js/macos-gettext-build, was " Johannes Schindelin @ 2019-05-06 9:11 ` Duy Nguyen 2019-05-07 2:36 ` Junio C Hamano 4 siblings, 1 reply; 38+ messages in thread From: Duy Nguyen @ 2019-05-06 9:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List On Thu, Apr 25, 2019 at 8:15 PM Junio C Hamano <gitster@pobox.com> wrote: > * nd/config-move-to (2019-01-14) 7 commits > - config.h: fix hdr-check warnings > - config: add --move-to > - config: factor out set_config_source_file() > - config: use OPT_FILENAME() > - config.c: add repo_config_set_worktree_gently() > - worktree.c: add get_worktree_config() > - config.c: avoid git_path() in do_git_config_sequence() > > Needs review. Please drop this for now. I still want to make it work (because I need it). But I'll try again with a full series of submodule/worktree support. Hopefully that will catch some reviewer's attention. -- Duy ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: What's cooking in git.git (Apr 2019, #05; Thu, 25) 2019-05-06 9:11 ` Duy Nguyen @ 2019-05-07 2:36 ` Junio C Hamano 0 siblings, 0 replies; 38+ messages in thread From: Junio C Hamano @ 2019-05-07 2:36 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > On Thu, Apr 25, 2019 at 8:15 PM Junio C Hamano <gitster@pobox.com> wrote: >> * nd/config-move-to (2019-01-14) 7 commits >> - config.h: fix hdr-check warnings >> - config: add --move-to >> - config: factor out set_config_source_file() >> - config: use OPT_FILENAME() >> - config.c: add repo_config_set_worktree_gently() >> - worktree.c: add get_worktree_config() >> - config.c: avoid git_path() in do_git_config_sequence() >> >> Needs review. > > Please drop this for now. I still want to make it work (because I need > it). But I'll try again with a full series of submodule/worktree > support. Hopefully that will catch some reviewer's attention. Will do; thanks. ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2019-05-08 1:59 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-25 10:15 What's cooking in git.git (Apr 2019, #05; Thu, 25) Junio C Hamano 2019-04-25 14:50 ` Elijah Newren 2019-04-25 22:16 ` js/partial-clone-connectivity-check (was: What's cooking in git.git (Apr 2019, #05; Thu, 25)) Josh Steadmon 2019-05-02 2:52 ` Jeff King 2019-05-02 16:48 ` Josh Steadmon 2019-05-02 21:45 ` Jeff King 2019-05-02 22:24 ` Jeff King 2019-05-06 19:16 ` [PATCH] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason 2019-05-06 20:24 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 0/2] perf-lib.sh: fix 0baf78e7bc regression, refactor & fix bugs Ævar Arnfjörð Bjarmason 2019-05-07 7:06 ` Jeff King 2019-05-07 10:54 ` [PATCH v3 0/6] " Ævar Arnfjörð Bjarmason 2019-05-07 21:36 ` Jeff King 2019-05-08 1:59 ` Junio C Hamano 2019-05-07 10:54 ` [PATCH v3 1/6] perf README: correct docs for 3c8f12c96c regression Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 2/6] perf aggregate: remove GIT_TEST_INSTALLED from --codespeed Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 3/6] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 4/6] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 5/6] perf tests: add "bindir" prefix to git tree test results Ævar Arnfjörð Bjarmason 2019-05-07 10:54 ` [PATCH v3 6/6] perf-lib.sh: forbid the use of GIT_TEST_INSTALLED Ævar Arnfjörð Bjarmason 2019-05-06 23:23 ` [PATCH v2 1/2] perf-lib.sh: make "./run <revisions>" use the correct gits Ævar Arnfjörð Bjarmason 2019-05-07 7:19 ` Jeff King 2019-05-06 23:23 ` [PATCH v2 2/2] perf-lib.sh: remove GIT_TEST_INSTALLED from perf-lib.sh Ævar Arnfjörð Bjarmason 2019-05-07 7:16 ` Jeff King 2019-05-07 8:31 ` Ævar Arnfjörð Bjarmason 2019-05-07 21:23 ` Jeff King 2019-04-26 5:05 ` What's cooking in git.git (Apr 2019, #05; Thu, 25) Taylor Blau 2019-04-26 5:41 ` Junio C Hamano 2019-04-26 5:53 ` Taylor Blau 2019-04-26 6:01 ` Junio C Hamano 2019-04-26 17:58 ` Kaartic Sivaraam 2019-04-27 5:06 ` Junio C Hamano 2019-04-27 5:57 ` Kaartic Sivaraam 2019-05-02 3:09 ` Jeff King 2019-04-29 22:20 ` js/macos-gettext-build, was " Johannes Schindelin 2019-05-05 5:13 ` Junio C Hamano 2019-05-06 9:11 ` Duy Nguyen 2019-05-07 2:36 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.