[pull] master from git:master #152
Merged
+514
−722
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using the perf suite's "run" helper in a vanilla build fails like this: $ make && (cd t/perf && ./run p0000-perf-lib-sanity.sh) === Running 1 tests in this tree === perf 1 - test_perf_default_repo works: 1 2 3 ok perf 2 - test_checkout_worktree works: 1 2 3 ok ok 3 - test_export works perf 4 - export a weird var: 1 2 3 ok perf 5 - éḿíẗ ńöń-ÁŚĆÍÍ ćḧáŕáćẗéŕś: 1 2 3 ok ok 6 - test_export works with weird vars perf 7 - important variables available in subshells: 1 2 3 ok perf 8 - test-lib-functions correctly loaded in subshells: 1 2 3 ok # passed all 8 test(s) 1..8 cannot open test-results/p0000-perf-lib-sanity.subtests: No such file or directory at ./aggregate.perl line 159. It is trying to aggregate results written into t/perf/test-results, but the p0000 script did not write anything there. The "run" script looks in $TEST_OUTPUT_DIRECTORY/test-results, or if that variable is not set, in test-results in the current working directory (which should be t/perf itself). It pulls the value of $TEST_OUTPUT_DIRECTORY from the GIT-BUILD-OPTIONS file. But that doesn't quite match the setup in perf-lib.sh (which is what scripts like p0000 use). There we do this at the top of the script: TEST_OUTPUT_DIRECTORY=$(pwd) and then let test-lib.sh append "/test-results" to that. Historically, that made the vanilla case work: we'd always use t/perf/test-results. But when $TEST_OUTPUT_DIRECTORY was set, it would break. Commit 5756ccd (t/perf: fix benchmarks with out-of-tree builds, 2025-04-28) fixed that second case by loading GIT-BUILD-OPTIONS ourselves. But that broke the vanilla case! Now our setting of $TEST_OUTPUT_DIRECTORY in perf-lib.sh is ignored, because it is overwritten by GIT-BUILD-OPTIONS. And when test-lib.sh sees that the output directory is empty, it defaults to t/test-results, rather than t/perf/test-results. Nobody seems to have noticed, probably for two reasons: 1. It only matters if you're trying to aggregate results (like the "run" script does). Just running "./p0000-perf-lib-sanity.sh" manually still produces useful output; the stored result files are just in an unexpected place. 2. There might be leftover files in t/perf/test-results from previous runs (before 5756ccd). In particular, the ".subtests" files don't tend to change, and the lack of that file is what causes it to barf completely. So it's possible that the aggregation could have been showing stale results that did not match the run that just happened. We can fix it by setting TEST_OUTPUT_DIRECTORY only after we've loaded GIT-BUILD-OPTIONS, so that we override its value and not the other way around. And we'll do so only when the variable is not set, which should retain the fix for that case from 5756ccd. Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
If you run: GIT_PERF_LARGE_REPO=/some/path ./p1006-cat-file.sh it will use the repo in /some/path. But if you use the "run" helper script to aggregate and compare results, like this: GIT_PERF_LARGE_REPO=/some/path ./run HEAD^ HEAD p1006-cat-file.sh it will ignore that variable. This is because the presence of the LARGE_REPO variable in GIT-BUILD-OPTIONS overrides what's in the environment. This started with 4638e88 (Makefile: use common template for GIT-BUILD-OPTIONS, 2024-12-06), which now writes even empty variables (though arguably it was wrong even before with a non-empty value, as we generally prefer the environment to take precedence over on-disk config). We had the same problem in perf-lib.sh itself, and we hacked around it with 32b74b9 (perf: do allow `GIT_PERF_*` to be overridden again, 2025-04-04). That's what lets the direct invocation of "./p1006" work above. And in fact that was sufficient for "./run", too, until it started loading GIT-BUILD-OPTIONS itself in 5756ccd (t/perf: fix benchmarks with out-of-tree builds, 2025-04-28). Now it has the same problem: it clobbers any incoming GIT_PERF options from the environment. We can use the same hack here in the "run" script. It's quite ugly, but it's just short enough that I don't think it's worth trying to factor it out into a common shell library. In the long run, we might consider teaching GIT-BUILD-OPTIONS to be more gentle in overwriting existing entries. There are probably other GIT_TEST_* variables which would need the same treatment. And if and when we come up with a more complete solution, we can use it in both spots. Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Commit 8002e8e (builtin/cat-file: use bitmaps to efficiently filter by object type, 2025-04-02) introduced a performance regression when we are not filtering objects: it uses bitmaps even when they won't help, incurring extra costs. For example, running the new perf tests from this commit, which check the performance of listing objects by oid: $ export GIT_PERF_LARGE_REPO=/path/to/linux.git $ git -C "$GIT_PERF_LARGE_REPO" repack -adb $ GIT_SKIP_TESTS=p1006.1 ./run 8002e8e^ 8002e8e p1006-cat-file.sh [...] Test 8002e8e^ 8002e8e ------------------------------------------------------------------------------- 1006.2: list all objects (sorted) 1.48(1.44+0.04) 6.39(6.35+0.04) +331.8% 1006.3: list all objects (unsorted) 3.01(2.97+0.04) 3.40(3.29+0.10) +13.0% 1006.4: list blobs 4.85(4.67+0.17) 1.68(1.58+0.10) -65.4% An invocation that filters, like listing all blobs (1006.4), does benefit from using the bitmaps; it now doesn't have to check the type of each object from the pack data, so the tradeoff is worth it. But for listing all objects in sorted idx order (1006.2), we otherwise would never open the bitmap nor the revindex file. Worse, our sorting step gets much worse. Normally we append into an array in pack .idx order, and the sort step is trivial. But with bitmaps, we get the objects in pack order, which is apparently random with respect to oid, and have to sort the whole thing. (Note that this freshly-packed state represents the best case for .idx sorting; if we had two packs, then we'd have their objects one after the other and qsort would have to interleave them). The unsorted test in 1006.3 is interesting: there we are going in pack order, so we load the revindex for the pack anyway. And though we don't sort the result, we do use an oidset to check for duplicates. So we can see in the 8002e8e^ timings that those two things cost ~1.5s over the sorted case (mostly the oidset hash cost). We also incur the extra cost to open the bitmap file as of 8002e8e, which seems to be ~400ms. (This would probably be faster with a bitmap lookup table, but writing that out is not yet the default). So we know that bitmaps help when there's filtering to be done, but otherwise make things worse. Let's only use them when there's a filter. The perf script shows that we've fixed the regressions without hurting the bitmap case: Test 8002e8e^ 8002e8e HEAD -------------------------------------------------------------------------------------------------------- 1006.2: list all objects (sorted) 1.56(1.53+0.03) 6.44(6.37+0.06) +312.8% 1.62(1.54+0.06) +3.8% 1006.3: list all objects (unsorted) 3.04(2.98+0.06) 3.45(3.38+0.07) +13.5% 3.04(2.99+0.04) +0.0% 1006.4: list blobs 5.14(4.98+0.15) 1.76(1.68+0.06) -65.8% 1.73(1.64+0.09) -66.3% Note that there's another related case: we might have a filter that cannot be used with bitmaps. That check is handled already for us in for_each_bitmapped_object(), though we'd still load the bitmap and revindex files pointlessly in that case. I don't think it can happen in practice for cat-file, though, since it allows only blob:none, blob:limit, and object:type filters, all of which work with bitmaps. It would be easy-ish to insert an extra check like: can_filter_bitmap(&opt->objects_filter); into the conditional, but I didn't bother here. It would be redundant with the call in for_each_bitmapped_object(), and the can_filter helper function is static local in the bitmap code (so we'd have to make it public). Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
This test indirectly checks that the lost-found folder has 2 files in it and then checks that the expected two files exist. Make this more deliberate by removing the old test -f and compare the actual ls of the lost-found directory with the expected files. Signed-off-by: Andrew Chitester <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
These bullet points are full-fledged paragraphs with sentences. It’s best to restrict semicolon-termination to the case when the bullet list amounts to a list of items.[1] † 1: Like “List: ... • first; ... • second; and ... • third.” Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Git versions are always capitalized. Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
The “Description” section decided to introduce and use the term “patch ID” for the ID value itself. Let’s use the same term on the options as well. Also make to sure to use bare “ID” instead of “id”. Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
You specifically need `--patch` since the default output is a raw diff. Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
The default `--unstable` is a legacy format that predates `--stable`. That’s why 2871f4d (builtin: patch-id: add --verbatim as a command mode, 2022-10-24) made `--verbatim` lock in[1] `--stable`: Users of --unstable mainly care about compatibility with old git versions, which unstripping the whitespace would break. Thus there isn't a usecase for the combination of --verbatim and --unstable, and we don't expose this so as to not add maintainence burden. † 1: imply `--stable`, disallow `--unstable` Signed-off-by: Kristoffer Haugsbakk <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
- Use _<placeholder>_ instead of <placeholder> in the description - Modify some samples to use <placeholders> - Use `backticks` for keywords and more complex option descriptions. The new rendering engine will apply synopsis rules to these spans. Signed-off-by: Michael Lyons <[email protected]> Acked-by: Jean-Noël AVILA <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
- Use _<placeholder>_ instead of <placeholder> in the description - Use _underscores_ around math associated with <placeholders> - Use `backticks` for keywords and more complex option descriptions. The new rendering engine will apply synopsis rules to these spans. Signed-off-by: Michael Lyons <[email protected]> Acked-by: Jean-Noël AVILA <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Git 2.51 learned how to import and export stashes. This is a secure and robust way to transfer working tree states across machines and comes with almost none of the pitfalls of rsync or other tools. Recommend this as an alternative in the FAQ. Signed-off-by: brian m. carlson <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Replace old-style `test -[df]` and `! test -[df]` assertions with the modern `test_path_is_file`, `test_path_is_dir`, and `test_path_is_missing` helpers. These helpers provide more informative error messages in case of failure (e.g., "File 'foo' is missing" instead of just exit code 1). While at it, fix a typo and an incorrect path reference in one of the test descriptions. Signed-off-by: K Jayatheerth <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
The documentation for the builtin API was moved from the technical documentation and into a comment in builtin.h by ec14d4e (builtin.h: take over documentation from api-builtin.txt, 2017-08-02). This documentation wasn't updated as part of the major overhaul to include a repository struct in 9b1cb50 (builtin: add a repository parameter for builtin functions, 2024-09-13). There was a brief update regarding the move from *.txt to *.adoc by e801522 (builtin.h: *.txt -> *.adoc fixes, 2025-03-03). I noticed that there was quite a bit missing from the old documentation, which is still visible on git-scm.com [1]. [1] git/git-scm.com#2124 This change updates the documentation in the following ways: 1. Updates the cmd_foo() prototype to include a repository. 2. Adds some newlines to have uniformity in the list of flags. 3. Adds a description of the NO_PARSEOPT flag. 4. Describes the tests that perform checks on all builtins, which may trip up a contributor working on a new builtin. I double-checked these instructions against a toy example in my local branch to be sure that it was complete. Signed-off-by: Derrick Stolee <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Perf-test fixes. * jk/t-perf-fixes: t/perf/run: preserve GIT_PERF_* from environment t/perf/perf-lib: fix assignment of TEST_OUTPUT_DIRECTORY
Fix for a performance regression in "git cat-file". * jk/cat-file-avoid-bitmap-when-unneeded: cat-file: only use bitmaps when filtering
Test update. * ac/t1420-use-more-direct-check: t1420: modernize the lost-found test
Update in-code comment doc to match the current API. * ds/builtin-doc-update: builtin.h: update documentation
Test update. * kj/t7101-modernize: t7101: modernize test path checks
Update a FAQ entry on synching two separate repositories using the "git stash export/import" recently introduced. * bc/doc-stash-import-export: gitfaq: document using stash import/export to sync working tree
"git patch-id" documentation updates. * kh/doc-patch-id: doc: patch-id: --verbatim locks in --stable doc: patch-id: spell out the git-diff-tree(1) form doc: patch-id: use definite article for the result patch-id: use “patch ID” throughout doc: patch-id: capitalize Git version doc: patch-id: don’t use semicolon between bullet points
Doc mark-up update. * ml/doc-blame-markup: doc: git-blame: convert to new doc format doc: blame-options: convert to new doc format
Signed-off-by: Junio C Hamano <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )