Junio C Hamano [Wed, 23 Nov 2022 02:22:23 +0000 (11:22 +0900)]
Merge branch 'rp/maintenance-qol'
'git maintenance register' is taught to write configuration to an
arbitrary path, and 'git for-each-repo' is taught to expand tilde
characters in paths.
* rp/maintenance-qol:
builtin/gc.c: fix use-after-free in maintenance_unregister()
maintenance --unregister: fix uninit'd data use & -Wdeclaration-after-statement
maintenance: add option to register in a specific config
for-each-repo: interpolate repo path arguments
Junio C Hamano [Wed, 23 Nov 2022 02:22:23 +0000 (11:22 +0900)]
Merge branch 'ab/coccicheck-incremental'
"make coccicheck" is time consuming. It has been made to run more
incrementally.
* ab/coccicheck-incremental:
Makefile: don't create a ".build/.build/" for cocci, fix output
spatchcache: add a ccache-alike for "spatch"
cocci: run against a generated ALL.cocci
cocci rules: remove <id>'s from rules that don't need them
Makefile: copy contrib/coccinelle/*.cocci to build/
cocci: optimistically use COMPUTE_HEADER_DEPENDENCIES
cocci: make "coccicheck" rule incremental
cocci: split off "--all-includes" from SPATCH_FLAGS
cocci: split off include-less "tests" from SPATCH_FLAGS
Makefile: split off SPATCH_BATCH_SIZE comment from "cocci" heading
Makefile: have "coccicheck" re-run if flags change
Makefile: add ability to TAB-complete cocci *.patch rules
cocci rules: remove unused "F" metavariable from pending rule
Makefile + shared.mak: rename and indent $(QUIET_SPATCH_T)
Junio C Hamano [Wed, 23 Nov 2022 02:22:23 +0000 (11:22 +0900)]
Merge branch 'es/chainlint-output'
Teach chainlint.pl to annotate the original test definition instead
of the token stream.
* es/chainlint-output:
chainlint: annotate original test definition rather than token stream
chainlint: latch start/end position of each token
chainlint: tighten accuracy when consuming input stream
chainlint: add explanatory comments
Junio C Hamano [Wed, 23 Nov 2022 02:22:22 +0000 (11:22 +0900)]
Merge branch 'dd/bisect-helper-subcommand'
Fix a regression in the bisect-helper which mistakenly treats
arguments to the command given to 'git bisect run' as arguments to
the helper.
* dd/bisect-helper-subcommand:
bisect--helper: parse subcommand with OPT_SUBCOMMAND
bisect--helper: move all subcommands into their own functions
bisect--helper: remove unused options
Junio C Hamano [Wed, 23 Nov 2022 02:22:22 +0000 (11:22 +0900)]
Merge branch 'ab/submodule-helper-prep-only'
Preparation to remove git-submodule.sh and replace it with a builtin.
* ab/submodule-helper-prep-only:
submodule--helper: use OPT_SUBCOMMAND() API
submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
submodule--helper: remove --prefix from "absorbgitdirs"
submodule API & "absorbgitdirs": remove "----recursive" option
submodule.c: refactor recursive block out of absorb function
submodule tests: test for a "foreach" blind-spot
submodule--helper: fix a memory leak in "status"
submodule tests: add tests for top-level flag output
submodule--helper: move "config" to a test-tool
Taylor Blau [Fri, 18 Nov 2022 23:43:56 +0000 (18:43 -0500)]
Merge branch 'vd/skip-cache-tree-update'
Avoid calling 'cache_tree_update()' when doing so would be redundant.
* vd/skip-cache-tree-update:
rebase: use 'skip_cache_tree_update' option
read-tree: use 'skip_cache_tree_update' option
reset: use 'skip_cache_tree_update' option
unpack-trees: add 'skip_cache_tree_update' option
cache-tree: add perf test comparing update and prime
Taylor Blau [Fri, 18 Nov 2022 23:43:07 +0000 (18:43 -0500)]
Merge branch 'ab/sha-makefile-doc'
Makefile comments updates and reordering to clarify knobs used to
choose SHA implementations.
* ab/sha-makefile-doc:
Makefile: discuss SHAttered in *_SHA{1,256} discussion
Makefile: document default SHA-1 backend on OSX
Makefile & test-tool: replace "DC_SHA1" variable with a "define"
Makefile: document SHA-1 and SHA-256 default and selection order
Makefile: document default SHA-256 backend
Makefile: rephrase the discussion of *_SHA1 knobs
Makefile: create and use sections for "define" flag listing
Makefile: correct DC_SHA1 documentation
INSTALL: remove discussion of SHA-1 backends
Makefile: always (re)set DC_SHA1 on fallback
t7610: use "file:///dev/null", not "/dev/null", fixes MinGW
On MinGW the "/dev/null" is translated to "nul" on command-lines, even
though as in this case it'll never end up referring to an actual file.
So on Windows the fix for the previous "example.com" timeout issue in 8354cf752ec (t7610: fix flaky timeout issue, don't clone from
example.com, 2022-11-05) would yield:
fatal: repo URL: 'nul' must be absolute or begin with ./|../
Let's evade this yet again by prefixing this with "file://", which
makes this pass in the Windows CI.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Taylor Blau [Tue, 15 Nov 2022 18:53:17 +0000 (13:53 -0500)]
builtin/gc.c: fix use-after-free in maintenance_unregister()
While trying to fix a move based on an uninitialized value (along with a
declaration after the first statement), be0fd57228
(maintenance --unregister: fix uninit'd data use &
-Wdeclaration-after-statement, 2022-11-15) unintentionally introduced a
use-after-free.
The problem arises when `maintenance_unregister()` sees a non-NULL
`config_file` string and thus tries to call
git_configset_get_value_multi() to lookup the corresponding values.
We store the result off, and then call git_configset_clear(), which
frees the pointer that we just stored. We then try to read that
now-freed pointer a few lines below, and there we have our
use-after-free:
$ ./t7900-maintenance.sh -vxi --run=23 --valgrind
[...]
+ git maintenance unregister --config-file ./other
==3048727== Invalid read of size 8
==3048727== at 0x1869CA: maintenance_unregister (gc.c:1590)
==3048727== by 0x188F42: cmd_maintenance (gc.c:2651)
==3048727== by 0x128C62: run_builtin (git.c:466)
==3048727== by 0x12907E: handle_builtin (git.c:721)
==3048727== by 0x1292EC: run_argv (git.c:788)
==3048727== by 0x12988E: cmd_main (git.c:926)
==3048727== by 0x21ED39: main (common-main.c:57)
==3048727== Address 0x4b38bc8 is 24 bytes inside a block of size 64 free'd
==3048727== at 0x484617B: free (vg_replace_malloc.c:872)
==3048727== by 0x2D207E: free_individual_entries (hashmap.c:188)
==3048727== by 0x2D2153: hashmap_clear_ (hashmap.c:207)
==3048727== by 0x270B5C: git_configset_clear (config.c:2375)
==3048727== by 0x1869AC: maintenance_unregister (gc.c:1585)
==3048727== by 0x188F42: cmd_maintenance (gc.c:2651)
==3048727== by 0x128C62: run_builtin (git.c:466)
==3048727== by 0x12907E: handle_builtin (git.c:721)
==3048727== by 0x1292EC: run_argv (git.c:788)
==3048727== by 0x12988E: cmd_main (git.c:926)
==3048727== by 0x21ED39: main (common-main.c:57)
[...]
Resolve this via a partial-revert of be0fd57228. The config_set struct
now gets a zero initialization, which makes free()-ing it a noop even
without calling git_configset_init(). When we do initialize it to a
non-zero value, it is only free()'d after our last read of `list`.
maintenance --unregister: fix uninit'd data use & -Wdeclaration-after-statement
Since (maintenance: add option to register in a specific config,
2022-11-09) we've been unable to build with "DEVELOPER=1" without
"DEVOPTS=no-error", as the added code triggers a
"-Wdeclaration-after-statement" warning.
And worse than that, the data handed to git_configset_clear() is
uninitialized, as can be spotted with e.g.:
./t7900-maintenance.sh -vixd --run=23 --valgrind
[...]
+ git maintenance unregister --force
Conditional jump or move depends on uninitialised value(s)
at 0x6B5F1E: git_configset_clear (config.c:2367)
by 0x4BA64E: maintenance_unregister (gc.c:1619)
by 0x4BD278: cmd_maintenance (gc.c:2650)
by 0x409905: run_builtin (git.c:466)
by 0x40A21C: handle_builtin (git.c:721)
by 0x40A58E: run_argv (git.c:788)
by 0x40AF68: cmd_main (git.c:926)
by 0x5D39FE: main (common-main.c:57)
Uninitialised value was created by a stack allocation
at 0x4BA22C: maintenance_unregister (gc.c:1557)
Let's fix both of these issues, and also move the scope of the
variable to the "if" statement it's used in, to make it obvious where
it's used.
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Ronan Pigott [Wed, 9 Nov 2022 19:07:08 +0000 (12:07 -0700)]
maintenance: add option to register in a specific config
maintenance register currently records the maintenance repo exclusively
within the user's global configuration, but other configuration files
may be relevant when running maintenance if they are included from the
global config. This option allows the user to choose where maintenance
repos are recorded.
Signed-off-by: Ronan Pigott <ronan@rjp.ie> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Ronan Pigott [Wed, 9 Nov 2022 19:07:07 +0000 (12:07 -0700)]
for-each-repo: interpolate repo path arguments
This is a quality of life change for git-maintenance, so repos can be
recorded with the tilde syntax. The register subcommand will not record
repos in this format by default.
Signed-off-by: Ronan Pigott <ronan@rjp.ie> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Glen Choo [Fri, 11 Nov 2022 22:35:06 +0000 (22:35 +0000)]
http: redact curl h2h3 headers in info
With GIT_TRACE_CURL=1 or GIT_CURL_VERBOSE=1, sensitive headers like
"Authorization" and "Cookie" get redacted. However, since [1], curl's
h2h3 module (invoked when using HTTP/2) also prints headers in its
"info", which don't get redacted. For example,
Teach http.c to check for h2h3 headers in info and redact them using the
existing header redaction logic. This fixes the broken redaction logic
that we noted in the previous commit, so mark the redaction tests as
passing under HTTP2.
Jeff King [Fri, 11 Nov 2022 22:35:05 +0000 (22:35 +0000)]
t: run t5551 tests with both HTTP and HTTP/2
We have occasionally seen bugs that affect Git running only against an
HTTP/2 web server, not an HTTP one. For instance, b66c77a64e (http:
match headers case-insensitively when redacting, 2021-09-22). But since
we have no test coverage using HTTP/2, we only uncover these bugs in the
wild.
That commit gives a recipe for converting our Apache setup to support
HTTP/2, but:
- it's not necessarily portable
- we don't want to just test HTTP/2; we really want to do a variety of
basic tests for _both_ protocols
This patch handles both problems by running a duplicate of t5551
(labeled as t5559 here) with an alternate-universe setup that enables
HTTP/2. So we'll continue to run t5551 as before, but run the same
battery of tests again with HTTP/2. If HTTP/2 isn't supported on a given
platform, then t5559 should bail during the webserver setup, and
gracefully skip all tests (unless GIT_TEST_HTTPD has been changed from
"auto" to "yes", where the point is to complain when webserver setup
fails).
In theory other http-related test scripts could benefit from the same
duplication, but doing t5551 should give us a reasonable check of basic
functionality, and would have caught both bugs we've seen in the wild
with HTTP/2.
A few notes on the implementation:
- a script enables the server side config by calling enable_http2
before starting the webserver. This avoids even trying to load any
HTTP/2 config for t5551 (which is what lets it keep working with
regular HTTP even on systems that don't support it). This also sets
a prereq which can be used by individual tests.
- As discussed in b66c77a64e, the http2 module isn't compatible with
the "prefork" mpm, so we need to pick something else. I chose
"event" here, which works on my Debian system, but it's possible
there are platforms which would prefer something else. We can adjust
that later if somebody finds such a platform.
- The test "large fetch-pack requests can be sent using chunked
encoding" makes sure we use a chunked transfer-encoding by looking
for that header in the trace. But since HTTP/2 has its own streaming
mechanisms, we won't find such a header. We could skip the test
entirely by marking it with !HTTP2. But there's some value in making
sure that the fetch itself succeeded. So instead, we'll confirm that
either we're using HTTP2 _or_ we saw the expected chunked header.
- the redaction tests fail under HTTP/2 with recent versions of curl.
This is a bug! I've marked them with !HTTP2 here to skip them under
t5559 for the moment. Using test_expect_failure would be more
appropriate, but would require a bunch of boilerplate. Since we'll
be fixing them momentarily, let's just skip them for now to keep the
test suite bisectable, and we can re-enable them in the commit that
fixes the bug.
- one alternative layout would be to push most of t5551 into a
lib-t5551.sh script, then source it from both t5551 and t5559.
Keeping t5551 intact seemed a little simpler, as its one less level
of indirection for people fixing bugs/regressions in the non-HTTP/2
tests.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com>
tests(scalar): tighten the stale `scalar.repo` test some
As pointed out by Stolee, the previous incarnation of this test case was
not stringent enough: we want to verify that _only_ the stale entries
are removed (previously, the test case would have succeeded even if all
entries had been removed).
Let's rectify this and verify that the other entries are left intact.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Remotes are considered "promisor" if extensions.partialClone and some
other configuration variables are set. The casing for this in
Documentation/technical/repository-version.txt is not proper and may
cause confusion. This change corrects this casing.
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Makefile: don't create a ".build/.build/" for cocci, fix output
Fix a couple of issues in the recently merged 0f3c55d4c2b (Merge
branch 'ab/coccicheck-incremental' into next, 2022-11-08):
In copying over the "contrib/coccinelle/" rules to
".build/contrib/coccinelle/" we inadvertently ended up with a
".build/.build/contrib/coccinelle/" as well. We'd generate the
per-file patches in the former, and keep the rule and overall result
in the latter. E.g. running:
make contrib/coccinelle/free.cocci.patch COCCI_SOURCES="attr.c grep.c"
Would, per "tree -a .build" yield the following result:
Now we'll instead generate all of our files in
".build/contrib/coccinelle/". Fixing this required renaming the
directory where we keep our per-file patches, as we'd otherwise
conflict with the result.
Now the per-file patch directory is named e.g. "free.cocci.d". And the
end result will now be:
The per-file patches now have a ".patch" file suffix, which fixes
another issue reported against 0f3c55d4c2b: The summary output was
confusing. Before for the "make" command above we'd emit:
I.e. we have an "SPATCH" line that makes it clear that we're running
against the "{attr,grep}.c" file. The "SPATCH CAT" is then altered to
correspond to it, showing that we're concatenating the
"free.cocci.d/**.patch" files into one generated "free.cocci.patch" at
the end.
Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
bisect--helper: parse subcommand with OPT_SUBCOMMAND
As of it is, we're parsing subcommand with OPT_CMDMODE, which will
continue to parse more options even if the command has been found.
When we're running "git bisect run" with a command that expecting
a "--log" or "--no-log" arguments, or one of those "--bisect-..."
arguments, bisect--helper may mistakenly think those options are
bisect--helper's option.
We may fix those problems by passing "--" when calling from
git-bisect.sh, and skip that "--" in bisect--helper. However, it may
interfere with user's "--".
Let's parse subcommand with OPT_SUBCOMMAND since that API was born for
this specific use-case.
Reported-by: Lukáš Doktor <ldoktor@redhat.com> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
'git-bisect.sh' used to have a 'bisect_next_check' to check if we have
both good/bad, old/new terms set or not. In commit 129a6cf344
(bisect--helper: `bisect_next_check` shell function in C, 2019-01-02),
a subcommand for bisect--helper was introduced to port the check to C.
Since d1bbbe45df (bisect--helper: reimplement `bisect_run` shell
function in C, 2021-09-13), all users of 'bisect_next_check' was
re-implemented in C, this subcommand was no longer used but we forgot
to remove '--bisect-next-check'.
'git-bisect.sh' also used to have a 'bisect_write' function, whose
third positional parameter was a "nolog" flag. This flag was only used
when 'bisect_start' invoked 'bisect_write' to write the starting good
and bad revisions. Then 0f30233a11 (bisect--helper: `bisect_write`
shell function in C, 2019-01-02) ported it to C as a command mode of
'bisect--helper', which (incorrectly) added the '--no-log' option,
and convert the only place ('bisect_start') that call 'bisect_write'
with 'nolog' to 'git bisect--helper --bisect-write' with 'nolog'
instead of '--no-log', since 'bisect--helper' has command modes not
subcommands, all other command modes see and handle that option as well.
This bogus state didn't last long, however, because in the same patch
series 06f5608c14 (bisect--helper: `bisect_start` shell function
partially in C, 2019-01-02) the C reimplementation of bisect_start()
started calling the bisect_write() C function, this time with the
right 'nolog' function parameter. From then on there was no need for
the '--no-log' option in 'bisect--helper'. Eventually all bisect
subcommands were ported to C as 'bisect--helper' command modes, each
calling the bisect_write() C function instead, but when the
'--bisect-write' command mode was removed in 68efed8c8a
(bisect--helper: retire `--bisect-write` subcommand, 2021-02-03) it
forgot to remove that '--no-log' option.
'--no-log' option had never been used and it's unused now.
Let's remove --bisect-next-check and --no-log from option parsing.
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Phillip Wood [Thu, 10 Nov 2022 16:43:41 +0000 (16:43 +0000)]
sequencer: tighten label lookups
The `label` command creates a ref refs/rewritten/<label> that the
`reset` and `merge` commands resolve by calling lookup_label(). That
uses lookup_commit_reference_by_name() to look up the label ref. As
lookup_commit_reference_by_name() uses the dwim rules when looking up
the label it will look for a branch named
refs/heads/refs/rewritten/<label> and return that instead of an error if
the branch exists and the label does not. Fix this by using read_ref()
followed by lookup_commit_object() when looking up labels.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Phillip Wood [Thu, 10 Nov 2022 16:43:40 +0000 (16:43 +0000)]
sequencer: unify label lookup
The arguments to the `reset` and `merge` commands may be a label created
with a `label` command or an arbitrary commit name. The `merge` command
uses the lookup_label() function to lookup its arguments but `reset` has
a slightly different version of that function in do_reset(). Reduce this
code duplication by calling lookup_label() from do_reset() as well.
This change improves the behavior of `reset` when the argument is a
tree. Previously `reset` would accept a tree only for the rebase to
fail with
Victoria Dye [Thu, 10 Nov 2022 19:06:05 +0000 (19:06 +0000)]
rebase: use 'skip_cache_tree_update' option
Enable the 'skip_cache_tree_update' option in both 'do_reset()'
('sequencer.c') and 'reset_head()' ('reset.c'). Both of these callers invoke
'prime_cache_tree()' after 'unpack_trees()', so we can remove an unnecessary
cache tree rebuild by skipping 'cache_tree_update()'.
When testing with 'p3400-rebase.sh' and 'p3404-rebase-interactive.sh', the
performance change of this update was negligible, likely due to the
operation being dominated by more expensive operations (like checking out
trees). However, since the change doesn't harm performance, it's worth
keeping this 'unpack_trees()' usage consistent with others that subsequently
invoke 'prime_cache_tree()'.
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Victoria Dye [Thu, 10 Nov 2022 19:06:04 +0000 (19:06 +0000)]
read-tree: use 'skip_cache_tree_update' option
When running 'read-tree' with a single tree and no prefix,
'prime_cache_tree()' is called after the tree is unpacked. In that
situation, skip a redundant call to 'cache_tree_update()' in
'unpack_trees()' by enabling the 'skip_cache_tree_update' unpack option.
Removing the redundant cache tree update provides a substantial performance
improvement to 'git read-tree <tree-ish>', as shown by a test added to
'p0006-read-tree-checkout.sh':
Test before after
----------------------------------------------------------------------
read-tree br_ballast_plus_1 3.94(1.80+1.57) 3.00(1.14+1.28) -23.9%
Note that the 'read-tree' in 't1022-read-tree-partial-clone.sh' is updated
to read two trees, rather than one. The test was first introduced in d3da223f221 (cache-tree: prefetch in partial clone read-tree, 2021-07-23) to
exercise the 'cache_tree_update()' code path, as used in 'git merge'. Since
this patch drops the call to 'cache_tree_update()' in single-tree 'git
read-tree', change the test to use the two-tree variant so that
'cache_tree_update()' is called as intended.
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Victoria Dye [Thu, 10 Nov 2022 19:06:03 +0000 (19:06 +0000)]
reset: use 'skip_cache_tree_update' option
Enable the 'skip_cache_tree_update' option in the variants that call
'prime_cache_tree()' after 'unpack_trees()' (specifically, 'git reset
--mixed' and 'git reset --hard'). This avoids redundantly rebuilding the
cache tree in both 'cache_tree_update()' at the end of 'unpack_trees()' and
in 'prime_cache_tree()', resulting in a small (but consistent) performance
improvement. From the newly-added 'p7102-reset.sh' test:
Test before after
--------------------------------------------------------------------
7102.1: reset --hard (...) 2.11(0.40+1.54) 1.97(0.38+1.47) -6.6%
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Victoria Dye [Thu, 10 Nov 2022 19:06:02 +0000 (19:06 +0000)]
unpack-trees: add 'skip_cache_tree_update' option
Add (disabled by default) option to skip the 'cache_tree_update()' at the
end of 'unpack_trees()'. In many cases, this cache tree update is redundant
because the caller of 'unpack_trees()' immediately follows it with
'prime_cache_tree()', rebuilding the entire cache tree from scratch. While
these operations aren't the most expensive part of operations like 'git
reset', the duplicate calls still create a minor unnecessary slowdown.
Introduce an option for callers to skip the 'cache_tree_update()' in
'unpack_trees()' if it is redundant (that is, if 'prime_cache_tree()' is
called afterwards). At the moment, no 'unpack_trees()' callers use the new
option; they will be updated in subsequent patches.
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Victoria Dye [Thu, 10 Nov 2022 19:06:01 +0000 (19:06 +0000)]
cache-tree: add perf test comparing update and prime
Add a performance test comparing the execution times of 'prime_cache_tree()'
and 'cache_tree_update(_, WRITE_TREE_SILENT | WRITE_TREE_REPAIR)'. The goal
of comparing these two is to identify which is the faster method for
rebuilding an invalid cache tree, ultimately to remove one when both are
(reundantly) called in immediate succession.
Both methods are fast, so the new tests in 'p0090-cache-tree.sh' must call
each tested function multiple times to ensure the reported times (to 0.01s
resolution) convey the differences between them.
The tests compare the timing of a 'test-tool cache-tree' run as a no-op (to
capture a baseline for the overhead associated with running the tool),
'cache_tree_update()', and 'prime_cache_tree()' on four scenarios:
- A completely valid cache tree
- A cache tree with 2 invalid paths
- A cache tree with 50 invalid paths
- A completely empty cache tree
These timings show that, while 'cache_tree_update()' is faster when the
cache tree is completely valid, it is equal to or slower than
'prime_cache_tree()' when there are any invalid paths. Since the redundant
calls are mostly in scenarios where the cache tree will be at least
partially invalid (e.g., 'git reset --hard'), 'prime_cache_tree()' will
likely perform better than 'cache_tree_update()' in typical cases.
Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Jeff King [Wed, 2 Nov 2022 05:27:49 +0000 (01:27 -0400)]
branch: gracefully handle '-d' on orphan HEAD
When deleting a branch, "git branch -d" has a safety check that ensures
the branch is merged to its upstream (if any), or to HEAD. To do that,
naturally we try to resolve HEAD to a commit object. If we're on an
orphan branch (i.e., HEAD points to a branch that does not yet exist),
that will fail, and we'll bail with an error:
$ git branch -d to-delete
fatal: Couldn't look up commit object for HEAD
This usually isn't that big of a deal. The deletion would fail anyway,
since the branch isn't merged to HEAD, and you'd need to use "-D" (or
"-f"). And doing so skips the HEAD resolution, courtesy of 67affd5173
(git-branch -D: make it work even when on a yet-to-be-born branch,
2006-11-24).
But there are still two problems:
1. The error message isn't very helpful. We should give the usual "not
fully merged" message, which points the user at "branch -D". That
was a problem even back in 67affd5173.
2. Even without a HEAD, these days it's still possible for the
deletion to succeed. After 67affd5173, commit 99c419c915 (branch
-d: base the "already-merged" safety on the branch it merges with,
2009-12-29) made it OK to delete a branch if it is merged to its
upstream.
We can fix both by removing the die() in delete_branches() completely,
leaving head_rev NULL in this case. It's tempting to stop there, as it
appears at first glance that the rest of the code does the right thing
with a NULL. But sadly, it's not quite true.
We end up feeding the NULL to repo_is_descendant_of(). In the
traditional code path there, we call repo_in_merge_bases_many(). It
feeds the NULL to repo_parse_commit(), which is smart enough to return
an error, and we immediately return "no, it's not a descendant".
But there's an alternate code path: if we have a commit graph with
generation numbers, we end up in can_all_from_reach(), which does
eventually try to set a flag on the NULL commit and segfaults.
So instead, we'll teach the local branch_merged() helper to treat a NULL
as "not merged". This would be a little more elegant in in_merge_bases()
itself, but that function is called in a lot of places, and it's not
clear that quietly returning "not merged" is the right thing everywhere
(I'd expect in many cases, feeding a NULL is a sign of a bug).
There are four tests here:
a. The first one confirms that deletion succeeds with an orphaned HEAD
when the branch is merged to its upstream. This is case (2) above.
b. Same, but with commit graphs enabled. Even if it is merged to
upstream, we still check head_rev so that we can say "deleting
because it's merged to upstream, even though it's not merged to
HEAD". Without the second hunk in branch_merged(), this test would
segfault in can_all_from_reach().
c. The third one confirms that we correctly say "not merged to HEAD"
when we can't resolve HEAD, and reject the deletion.
d. Same, but with commit graphs enabled. Without the first hunk in
branch_merged(), this one would segfault.
Reported-by: Martin von Zweigbergk <martinvonz@google.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com>
M Hickford [Wed, 9 Nov 2022 16:11:13 +0000 (16:11 +0000)]
Documentation: increase example cache timeout to 1 hour
Previously, the example *decreased* the cache timeout compared to the
default, making it less user friendly.
Instead, nudge users to make cache more usable. Many users choose
store over cache.
https://lore.kernel.org/git/CAGJzqskRYN49SeS8kSEN5-vbB_Jt1QvAV9QhS6zNuKh0u8wxPQ@mail.gmail.com/
The default timeout remains 15 minutes. A stronger nudge would
be to increase that.
Signed-off-by: M Hickford <mirth.hickford@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Phillip Wood [Wed, 9 Nov 2022 14:21:58 +0000 (14:21 +0000)]
rebase: stop exporting GIT_REFLOG_ACTION
Now that struct replay_opts has a reflog_action member we no longer
need to export GIT_REFLOG_ACTION when starting a rebase. If the user
has set GIT_REFLOG_ACTION then we use it when initializing
reflog_action.
Phillip Wood [Wed, 9 Nov 2022 14:21:57 +0000 (14:21 +0000)]
sequencer: stop exporting GIT_REFLOG_ACTION
Each time it picks a commit the sequencer copies the GIT_REFLOG_ACITON
environment variable so it can temporarily change it and then restore
the previous value. This results in code that is hard to follow and also
leaks memory because (i) we fail to free the copy when we've finished
with it and (ii) each call to setenv() leaks the previous value. Instead
pass the reflog action around in a variable and use it to set
GIT_REFLOG_ACTION in the child environment when running "git commit".
Within the sequencer GIT_REFLOG_ACTION is no longer set and is only read
by sequencer_reflog_action(). It is still set by rebase before calling
the sequencer, that will be addressed in the next commit. cherry-pick
and revert are unaffected as they do not set GIT_REFLOG_ACTION before
calling the sequencer.
t7610: fix flaky timeout issue, don't clone from example.com
When t7610-mergetool.sh runs without failures the git://example.com
submodule URLs will never be used. That's because we "git submodule
add" it, but then manually populate them so that subsequent "git
submodule update -N" won't attempt to clone it, only update it without
fetching.
But if we fail in an earlier test it'll have the knock-on effect of
having later tests hang on that "git submodule update -N" as we
attempt to clone this repository from example.com.
This can be reproduced on "master" by running the test with
SANITIZE=leak without "--immediate". With
"GIT_TEST_PASSING_SANITIZE_LEAK=true" (which the linux-leaks job uses)
we'll skip the test entirely. So we'll only run into this when running
it manually, or with the "GIT_TEST_PASSING_SANITIZE_LEAK=check" mode.
That's not because the failure has anything to do with leak detection
per-se. It just so happens that we have a leak that'll fail before
we've managed to fully set these up, and therefore "git submodule
update -N" ends up spawning "git clone".
Let's instead continue lying about the origin of this submodule by
providing a URL for it that doesn't work, but now one that *really*
doesn't work: /dev/null. If the test is passing we won't ever use
this, and if we have knock-on failures we'll fail early, instead of
waiting for a timeout.
The behavior of "-N" here might be surprising to some, since it's
explained as "[if you use -N we] don’t fetch new objects from the
remote site". But (perhaps counter-intuitively) it's only talking
about if it needs to do so via "git fetch". In this case we'll end up
spawning a "git clone", as we have no submodule set up.
See ff7f089ed10 (mergetool: Teach about submodules, 2011-04-13) for
the commit that implemented these "example.com" tests.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Taylor Blau [Tue, 8 Nov 2022 22:15:12 +0000 (17:15 -0500)]
Merge branch 'rs/no-more-run-command-v'
Simplify the run-command API.
* rs/no-more-run-command-v:
replace and remove run_command_v_opt()
replace and remove run_command_v_opt_cd_env_tr2()
replace and remove run_command_v_opt_tr2()
replace and remove run_command_v_opt_cd_env()
use child_process members "args" and "env" directly
use child_process member "args" instead of string array variable
sequencer: simplify building argument list in do_exec()
bisect--helper: factor out do_bisect_run()
bisect: simplify building "checkout" argument list
am: simplify building "show" argument list
run-command: fix return value comment
merge: remove always-the-same "verbose" arguments
Taylor Blau [Tue, 8 Nov 2022 22:14:52 +0000 (17:14 -0500)]
Merge branch 'jk/ref-filter-parsing-bugs'
Various tests exercising the transfer.credentialsInUrl configuration
are taught to avoid making requests which require resolving localhost
to reduce CI-flakiness.
* jk/ref-filter-parsing-bugs:
ref-filter: fix parsing of signatures with CRLF and no body
ref-filter: fix parsing of signatures without blank lines
revisions API: extend the nascent REV_INFO_INIT macro
Have the REV_INFO_INIT macro added in [1] declare more members of
"struct rev_info" that we can initialize statically, and have
repo_init_revisions() do so with the memcpy(..., &blank) idiom
introduced in [2].
As the comment for the "REV_INFO_INIT" macro notes this still isn't
sufficient to initialize a "struct rev_info" for use yet. But we are
getting closer to that eventual goal.
Even though we can't fully initialize a "struct rev_info" with
REV_INFO_INIT it's useful for readability to clearly separate those
things that we can statically initialize, and those that we can't.
This change could replace the:
list_objects_filter_init(&revs->filter);
In the repo_init_revisions() with this line, at the end of the
REV_INFO_INIT deceleration in revisions.h:
.filter = LIST_OBJECTS_FILTER_INIT, \
But doing so would produce a minor conflict with an outstanding
topic[3]. Let's skip that for now. I have follow-ups to initialize
more of this statically, e.g. changes to get rid of grep_init(). We
can initialize more members with the macro in a future series.
The old version we currently use runs in node.js v12.x, which is being
deprecated in GitHub Actions. The new version uses node.js v16.x.
Incidentally, this also avoids the warning about the deprecated
`::set-output::` workflow command because the newer version of the
`github-script` Action uses the recommended new way to specify outputs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Eric Sunshine [Tue, 8 Nov 2022 19:08:30 +0000 (19:08 +0000)]
chainlint: annotate original test definition rather than token stream
When chainlint detects problems in a test, such as a broken &&-chain, it
prints out the test with "?!FOO?!" annotations inserted at each problem
location. However, rather than annotating the original test definition,
it instead dumps out a parsed token representation of the test. Since it
lacks comments, indentations, here-doc bodies, and so forth, this
tokenized representation can be difficult for the test author to digest
and relate back to the original test definition.
However, now that each parsed token carries positional information, the
location of a detected problem can be pinpointed precisely in the
original test definition. Therefore, take advantage of this information
to annotate the test definition itself rather than annotating the parsed
token stream, thus making it easier for a test author to relate a
problem back to the source.
Maintaining the positional meta-information associated with each
detected problem requires a slight change in how the problems are
managed internally. In particular, shell syntax such as:
msg="total: $(cd data; wc -w *.txt) words"
requires the lexical analyzer to recursively invoke the parser in order
to detect problems within the $(...) expression inside the double-quoted
string. In this case, the recursive parse context will detect the broken
&&-chain between the `cd` and `wc` commands, returning the token stream:
cd data ; ?!AMP?! wc -w *.txt
However, the parent parse context will see everything inside the
double-quotes as a single string token:
"total: $(cd data ; ?!AMP?! wc -w *.txt) words"
losing whatever positional information was attached to the ";" token
where the problem was detected.
One way to preserve the positional information of a detected problem in
a recursive parse context within a string would be to attach the
positional information to the annotation textually; for instance:
"total: $(cd data ; ?!AMP:21:22?! wc -w *.txt) words"
and then extract the positional information when annotating the original
test definition.
However, a cleaner and much simpler approach is to maintain the list of
detected problems separately rather than embedding the problems as
annotations directly in the parsed token stream. Not only does this
ensure that positional information within recursive parse contexts is
not lost, but it keeps the token stream free from non-token pollution,
which may simplify implementation of validations added in the future
since they won't have to handle non-token "?!FOO!?" items specially.
Finally, the chainlint self-test "expect" files need a few mechanical
adjustments now that the original test definitions are emitted rather
than the parsed token stream. In particular, the following items missing
from the historic parsed-token output are now preserved verbatim:
* indentation (and whitespace, in general)
* comments
* here-doc bodies
* here-doc tag quoting (i.e. "\EOF")
* line-splices (i.e. "\" at the end of a line)
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Eric Sunshine [Tue, 8 Nov 2022 19:08:29 +0000 (19:08 +0000)]
chainlint: latch start/end position of each token
When chainlint detects problems in a test, such as a broken &&-chain, it
prints out the test with "?!FOO?!" annotations inserted at each problem
location. However, rather than annotating the original test definition,
it instead dumps out a parsed token representation of the test. Since it
lacks comments, indentations, here-doc bodies, and so forth, this
tokenized representation can be difficult for the test author to digest
and relate back to the original test definition.
To address this shortcoming, an upcoming change will make it print out
an annotated copy of the original test definition rather than the
tokenized representation. In order to do so, it will need to know the
start and end positions of each token in the original test definition.
As preparation, upgrade TestParser::scan_token() to latch the start and
end position of the token being scanned, and return that information
along with the token itself. A subsequent change will take advantage of
this positional information.
In terms of implementation, TestParser::scan_token() is retrofitted to
return a tuple consisting of the token's lexeme and its start and end
positions, rather than returning just the lexeme. However, an
alternative would be to define a class which represents a token:
package Token;
sub new {
my ($class, $lexeme, $start, $end) = @_;
bless [$lexeme, $start, $end] => $class;
}
sub as_string {
my $self = shift @_;
return $self->[0];
}
sub compare {
my ($x, $y) = @_;
if (UNIVERSAL::isa($y, 'Token')) {
return $x->[0] cmp $y->[0];
}
return $x->[0] cmp $y;
}
use overload (
'""' => 'as_string',
'cmp' => 'compare'
);
The major benefit of the class-based approach is that it is entirely
non-invasive; it requires no additional changes to the rest of the
script since a Token converts automatically to a string, which is what
scan_token() historically returned.
The big downside to the Token approach, however, is that it is _slow_;
on this developer's (old) machine, it increases user-time by an
unacceptable seven seconds when scanning all test scripts in the
project. Hence, the simple tuple approach is employed instead since it
adds only a fraction of a second user-time.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Eric Sunshine [Tue, 8 Nov 2022 19:08:28 +0000 (19:08 +0000)]
chainlint: tighten accuracy when consuming input stream
To extract the next token in the input stream, Lexer::scan_token() finds
the start of the token by skipping whitespace, then consumes characters
belonging to the token until it encounters a non-token character, such
as an operator, punctuation, or whitespace. In the case of an operator
or punctuation which ends a token, before returning the just-scanned
token, it pushes that operator or punctuation character back onto the
input stream to ensure that it will be the first character consumed by
the next call to scan_token().
However, scan_token() is intentionally lax when whitespace ends a token;
it doesn't bother pushing the whitespace character back onto the token
stream since it knows that the next call to scan_token() will, as its
first step, skip over whitespace anyhow when looking for the start of
the token.
Although such laxity is harmless for the proper functioning of the
lexical analyzer, it does make it difficult to precisely identify the
token's end position in the input stream. Accurate token position
information may be desirable, for instance, to annotate problems or
highlight other interesting facets of the input found during the parsing
phase. To accommodate such possibilities, tighten scan_token() by making
it push the token-ending whitespace character back onto the input
stream, just as it does for other token-ending characters.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Eric Sunshine [Tue, 8 Nov 2022 19:08:27 +0000 (19:08 +0000)]
chainlint: add explanatory comments
The logic in TestParser::accumulate() for detecting broken &&-chains is
mostly well-commented, but a couple branches which were deemed obvious
and straightforward lack comments. In retrospect, though, these cases
may give future readers pause, so comment them, as well.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API
introduced in fa83cc834da (parse-options: add support for parsing
subcommands, 2022-08-19).
This is only a marginal reduction in line count, but once we start
unifying this with a yet-to-be-added "builtin/submodule.c" it'll be
much easier to reason about those changes, as they'll both use
OPT_SUBCOMMAND().
We don't need to worry about "argv[0]" being NULL in the die() because
we'd have errored out in parse_options() as we're not using
"PARSE_OPT_SUBCOMMAND_OPTIONAL".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"
Since 29a5e9e1ffe (submodule--helper update-clone: learn --init,
2022-03-04) we've been passing "-C <prefix>" from "git-submodule.sh"
whenever we pass "--prefix <prefix>", so the latter is redundant to
the former. Let's drop the "--prefix" option.
submodule--helper: remove --prefix from "absorbgitdirs"
Let's pass the "-C <prefix>" option instead to "absorbgitdirs" from
its only caller.
When it was added in f6f85861400 (submodule: add absorb-git-dir
function, 2016-12-12) there were other "submodule--helper" subcommands
that were invoked with "-C <prefix>", so we could have done this all
along.
submodule API & "absorbgitdirs": remove "----recursive" option
Remove the "----recursive" option to "git submodule--helper
absorbgitdirs" (yes, with 4 dashes, not 2).
This option and all the "else" when "flags &
ABSORB_GITDIR_RECURSE_SUBMODULES" is false has never been used since
it was added in f6f85861400 (submodule: add absorb-git-dir function,
2016-12-12), which we'd have had to do as "----recursive", a
"--recursive" would have errored out.
It would be nice to follow-up with an optbug() assertion to
parse-options.c for such funnily named options, I manually validated
that this was the only long option whose name started with "-", but
let's skip adding such an assertion for now.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
submodule.c: refactor recursive block out of absorb function
A move and indentation-only change to move the
ABSORB_GITDIR_RECURSE_SUBMODULES case into its own function, which as
we'll see makes the subsequent commit changing this code much smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
The "status" sub-command was leaking the "struct strvec" it was
setting up for the reasons explained in f92dbdbc6a8 (revisions API:
don't leak memory on argv elements that need free()-ing, 2022-08-02),
so let's use the "free_removed_argv_elements" option to
setup_revisions() to fix the leak.
Even if we did that, clobbering the "diff_files_args.nr" with the
return value of setup_revisions() would leave leaks in place, but we
can just stop clobbering it.
Ever since that code was added in a9f8a37584a (submodule: port
submodule subcommand 'status' from shell to C, 2017-10-06) we've had
no reason to modify the "nr" member ("argc" at the time): The next use
of "diff_files_args" after this is the "strvec_clear()" at the end of
the function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
submodule tests: add tests for top-level flag output
Exhaustively test for how combining various "mixed-level" "git
submodule" option works. "Mixed-level" here means options that are
accepted by a mixture of the top-level "submodule" command, and
e.g. the "status" sub-command.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
As with other moves to "test-tool" in f322e9f51b5 (Merge branch
'ab/submodule-helper-prep', 2022-09-13) the "config" sub-command was
only used by our own tests.
It was last used by "git submodule" itself in code that went away with a6226fd772b (submodule--helper: convert the bulk of cmd_add() to C,
2021-08-10).
Let's move it over, and while doing so make it easier to reason about
by splitting up the various uses for it into separate sub-commands, so
that we don't need to count arguments to see what it does.
This also has the advantage that we stop wasting future translator
time on this command, currently the usage information for this
internal-only tool has been translated into several languages. The use
of the "_" function has also been removed from the "please make
sure..." message.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Makefile: discuss SHAttered in *_SHA{1,256} discussion
Let's mention the SHAttered attack and more generally why we use the
sha1collisiondetection backend by default, and note that for SHA-256
the user should feel free to pick any of the supported backends as far
as hashing security is concerned.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Since [1] the default SHA-1 backend on OSX has been
APPLE_COMMON_CRYPTO. Per [2] we'll skip using it on anything older
than Mac OS X 10.4 "Tiger"[3].
When "DC_SHA1" was made the default in [4] this interaction between it
and APPLE_COMMON_CRYPTO seems to have been missed in. Ever since
DC_SHA1 was "made the default" we've still used Apple's CommonCrypto
instead of sha1collisiondetection on modern versions of Darwin and
OSX.
1. 61067954ce1 (cache.h: eliminate SHA-1 deprecation warnings on Mac
OS X, 2013-05-19)
2. 9c7a0beee09 (config.mak.uname: set NO_APPLE_COMMON_CRYPTO on older
systems, 2014-08-15)
3. We could probably drop "NO_APPLE_COMMON_CRYPTO", as nobody's likely
to care about such on old version of OSX anymore. But let's leave that
for now.
4. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Makefile & test-tool: replace "DC_SHA1" variable with a "define"
Address the root cause of technical debt we've been carrying since
sha1collisiondetection was made the default in [1]. In a preceding
commit we narrowly fixed a bug where the "DC_SHA1" variable would be
unset (in combination with "NO_APPLE_COMMON_CRYPTO=" on OSX), even
though we had the sha1collisiondetection library enabled.
But the only reason we needed to have such a user-exposed knob went
away with [1], and it's been doing nothing useful since then. We don't
care if you define DC_SHA1=*, we only care that you don't ask for any
other SHA-1 implementation. If it turns out that you didn't, we'll use
sha1collisiondetection, whether you had "DC_SHA1" set or not.
As a result of this being confusing we had e.g. [2] for cmake and the
recent [3] for ci/lib.sh setting "DC_SHA1" explicitly, even though
this was always a NOOP.
A much simpler way to do this is to stop having the Makefile and
CMakeLists.txt set "DC_SHA1" to be picked up by the test-lib.sh, let's
instead add a trivial "test-tool sha1-is-sha1dc". It returns zero if
we're using sha1collisiondetection, non-zero otherwise.
1. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)
2. c4b2f41b5f5 (cmake: support for testing git with ctest, 2020-06-26)
3. 1ad5c3df35a (ci: use DC_SHA1=YesPlease on osx-clang job for CI,
2022-10-20)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Makefile: document SHA-1 and SHA-256 default and selection order
For the *_SHA1 and *_SHA256 flags we've discussed the various flags,
but not the fact that when you define multiple flags we'll pick one.
Which one we pick depends on the order they're listed in the Makefile,
which differed from the order we discussed them in this documentation.
Let's be explicit about how we select these, and re-arrange the
listings so that they're listed in the priority order we've picked.
I'd personally prefer that the selection was more explicit, and that
we'd error out if conflicting flags were provided, but per the
discussion downhtread of[1] the consensus was to keep theses semantics.
This behavior makes it easier to e.g. integrate with autoconf-like
systems, where the configuration can provide everything it can
support, and Git is tasked with picking the first one it prefers.
Since 27dc04c5450 (sha256: add an SHA-256 implementation using
libgcrypt, 2018-11-14) we've claimed to support a BLK_SHA256 flag, but
there's no such SHA-256 backend.
Instead we fall back on adding "sha256/block/sha256.o" to "LIB_OBJS"
and adding "-DSHA256_BLK" to BASIC_CFLAGS.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
In the preceding commit the discussion of the *_SHA1 knobs was left
as-is to benefit from a smaller diff, but since we're changing these
let's use the same phrasing we use for most other knobs. E.g. "define
X", not "define X environment variable", and get rid of the "when
running make to link with" entirely.
Furthermore the discussion of DC_SHA1* options is now under a "Options
for the sha1collisiondetection implementation" heading, so we don't
need to clarify that these options go along with DC_SHA1=Y, so let's
rephrase them accordingly.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Makefile: create and use sections for "define" flag listing
Since the "Define ..." template of comments at the top of the Makefile
was started in 5bdac8b3269 ([PATCH] Improve the compilation-time
settings interface, 2005-07-29) we've had a lot more flags added,
including flags that come in "groups". Not having any obvious
structure to the >500 line comment at the top of the Makefile has made
it hard to follow.
This change is almost entirely a move-only change, the two paragraphs
at the start of the first two sections are new, and so are the added
sections themselves, but other than that no lines are changed, only
moved.
We now list Makefile-only flags at the start, followed by stand-alone
flags, and then cover "optional library" flags in their respective
groups, followed by SHA-1 and SHA-256 flags, and finally
DEVELOPER-specific flags.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
The claim that DC_SHA1 takes priority over other *_SHA1 knobs was true
when it was added in [1], But that hasn't been the case since it was
made the fallback default in [2].
We should be making it not only the default, but something that takes
priority over other *_SHA1 knobs, but that's outside the scope of this
change. For now let's correct the documentation to match reality.
Let's also remove the "unconditionally enable" wording, per the above
the enabling of "DC_SHA1" is conditional on these other flags.
The "Define DC_SHA1" here is also a lie, actually it's "we don't care
if you define DC_SHA1, just don't define anything else", but that's a
more general issue that'll be addressed in a subsequent commit. Let's
first stop pretending that this setting (which we actually don't even
use) takes priority over anything else.
1. 8325e43b82d (Makefile: add DC_SHA1 knob, 2017-03-16)
2. e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
The claim that OpenSSL is the default SHA-1 backend hasn't been true
since e6b07da2780 (Makefile: make DC_SHA1 the default, 2017-03-17),
but more importantly tweaking the SHA-1 backend isn't something that's
common enough to warrant discussing in the INSTALL document, so let's
remove this paragraph.
This discussion was originally added in c538d2d34ab (Add some
installation notes in INSTALL, 2005-06-17) when tweaking the default
backend was more common. The current wording was added in 5beb577db8c (INSTALL: Describe dependency knobs from Makefile,
2009-09-10).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Fix an edge case introduced in in e6b07da2780 (Makefile: make DC_SHA1
the default, 2017-03-17), when DC_SHA1 was made the default fallback
we started unconditionally adding to BASIC_CFLAGS and LIB_OBJS, so
we'd use the sha1collisiondetection by default.
But the "DC_SHA1" variable remained unset, so e.g.:
make test DC_SHA1= T=t0013*.sh
Would skip the sha1collisiondetection tests, as we'd write
"DC_SHA1=''" to "GIT-BUILD-OPTIONS", but if we manually removed that
test prerequisite we'd pass the test (which we couldn't if we weren't
using sha1collisiondetection).
So let's have the fallback assignment use the 'override' directive
instead of the ":=" simply expanded variable introduced in e6b07da2780. In this case we explicitly want to override the user's
choice.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Elijah Newren [Sun, 6 Nov 2022 06:04:26 +0000 (06:04 +0000)]
sparse-checkout.txt: new document with sparse-checkout directions
Once upon a time, Matheus wrote some patches to make
git grep [--cached | <REVISION>] ...
restrict its output to the sparsity specification when working in a
sparse checkout[1]. That effort got derailed by two things:
(1) The --sparse-index work just beginning which we wanted to avoid
creating conflicts for
(2) Never deciding on flag and config names and planned high level
behavior for all commands.
More recently, Shaoxuan implemented a more limited form of Matheus'
patches that only affected --cached, using a different flag name,
but also changing the default behavior in line with what Matheus did.
This again highlighted the fact that we never decided on command line
flag names, config option names, and the big picture path forward.
The --sparse-index work has been mostly complete (or at least released
into production even if some small edges remain) for quite some time
now. We have also had several discussions on flag and config names,
though we never came to solid conclusions. Stolee once upon a time
suggested putting all these into some document in
Documentation/technical[3], which Victoria recently also requested[4].
I'm behind the times, but here's a patch attempting to finally do that.
[1] https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/
(See his second link in that email in particular)
[2] https://lore.kernel.org/git/20220908001854.206789-2-shaoxuan.yuan02@gmail.com/
[3] https://lore.kernel.org/git/CABPp-BHwNoVnooqDFPAsZxBT9aR5Dwk5D9sDRCvYSb8akxAJgA@mail.gmail.com/
(Scroll to the very end for the final few paragraphs)
[4] https://lore.kernel.org/git/cafcedba-96a2-cb85-d593-ef47c8c8397c@github.com/
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Victoria Dye [Mon, 7 Nov 2022 17:47:52 +0000 (09:47 -0800)]
rebase --update-refs: avoid unintended ref deletion
In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,
2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle
the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it
removes potential ref updates from the "update refs state" if a ref does not
have a corresponding 'update-ref' line.
However, because 'write_update_refs_state()' will not update the state if
the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will
result in the state remaining unchanged from how it was initialized (with
all refs' "after" OID being null). Then, when the ref update is applied, all
refs will be updated to null and consequently deleted.
To fix this, delete the 'update-refs' state file when 'refs_to_oids' is
empty. Additionally, add a tests covering "all update-ref lines removed"
cases.
Reported-by: herr.kaste <herr.kaste@gmail.com> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
scalar reconfigure -a: remove stale `scalar.repo` entries
Every once in a while, a Git for Windows installation fails because the
attempt to reconfigure a Scalar enlistment failed because it was deleted
manually without removing the corresponding entries in the global Git
config.
In f5f0842d0b5 (scalar: let 'unregister' handle a deleted enlistment
directory gracefully, 2021-12-03), we already taught `scalar delete` to
handle the case of a manually deleted enlistment gracefully. This patch
adds the same graceful handling to `scalar reconfigure --all`.
This patch is best viewed with `--color-moved`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Anh Le [Thu, 3 Nov 2022 23:05:01 +0000 (23:05 +0000)]
index: raise a bug if the index is materialised more than once
If clear_skip_worktree_from_present_files() encounter a sparse directory,
it fully materialise the index which should expand any sparse directories
and start going through each entries again. If this happens more than once,
raise it with a BUG.
Signed-off-by: Anh Le <anh@canva.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Anh Le [Thu, 3 Nov 2022 23:05:00 +0000 (23:05 +0000)]
index: add trace2 region for clear skip worktree
When using sparse checkout, clear_skip_worktree_from_present_files() must
enumerate index entries to find ones with the SKIP_WORKTREE bit to
determine whether those index entries exist on disk (in which case their
SKIP_WORKTREE bit should be removed).
In a large repository, this may take considerable time depending on the
size of the index.
Add a trace2 region to surface this information, keeping a count of how
many paths have been checked. Separately, keep counts after a full index is
materialized.
Signed-off-by: Anh Le <anh@canva.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Debra Obondo [Fri, 4 Nov 2022 15:05:52 +0000 (15:05 +0000)]
t7001-mv.sh: modernizing test script using functions
Test script to verify the presence/absence of files, paths, directories,
symlinks and other features in 'git mv' command are using the command
format:
'test (-e|f|d|h|...)'
Replace them with helper functions of format:
'test_path_is_*'
Replacing idiomatic helper functions:
'! test_path_is_*'
with
'test_path_is_missing'
This uses values of 'test_path_bar' in place of '! test_path_foo' to
bring in the helpful factor of indicating the failure of tests after the
mv command has been used, that is, it echoes if the feature/test_path
exists.
Signed-off-by: Debra Obondo <debraobondo@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com>
Taylor Blau [Fri, 4 Nov 2022 00:41:06 +0000 (20:41 -0400)]
Merge branch 'jk/avoid-localhost'
Various tests exercising the transfer.credentialsInUrl configuration
are taught to avoid making requests which require resolving localhost
to reduce CI-flakiness.
* jk/avoid-localhost:
t5516/t5601: be less strict about the number of credential warnings
t5516: move plaintext-password tests from t5601 and t5516
projects / thirdparty / git.git / log
