Derrick Stolee [Wed, 9 Mar 2022 16:01:32 +0000 (16:01 +0000)]
list-objects-filter-options: create copy helper
As we add more embedded members with type 'struct
list_objects_filter_options', it will be important to easily perform a
deep copy across multiple such structs. Create
list_objects_filter_copy() to satisfy this need.
This method is recursive to match the recursive nature of the struct.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 9 Mar 2022 16:01:31 +0000 (16:01 +0000)]
index-pack: document and test the --promisor option
The --promisor option of 'git index-pack' was created in 88e2f9e
(introduce fetch-object: fetch one promisor object, 2017-12-05) but was
untested. It is currently unused within the Git codebase, but that will
change in an upcoming change to 'git bundle unbundle' when there is a
filter capability.
For now, add documentation about the option and add a test to ensure it
is working as expected.
Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a meta's http-equiv equals "content-type", the http-equiv is said
to be in the "Encoding declaration state". According to the HTML
Standard,
The Encoding declaration state may be used in HTML documents,
but elements with an http-equiv attribute in that state must not
be used in XML documents.
Glen Choo [Tue, 8 Mar 2022 00:14:31 +0000 (16:14 -0800)]
submodule: move logic into fetch_task_create()
get_fetch_task() gets a fetch task by iterating the index; a future
commit will introduce a similar function, get_fetch_task_from_changed(),
that gets a fetch task from the list of changed submodules. Both
functions are similar in that they need to:
* create a fetch task
* initialize the submodule repo for the fetch task
* determine the default recursion mode
Move all of this logic into fetch_task_create() so that it is no longer
split between fetch_task_create() and get_fetch_task(). This will make
it easier to share code with get_fetch_task_from_changed().
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:30 +0000 (16:14 -0800)]
submodule: extract get_fetch_task()
get_next_submodule() configures the parallel submodule fetch by
performing two functions:
* iterate the index to find submodules
* configure the child processes to fetch the submodules found in the
previous step
Extract the index iterating code into an iterator function,
get_fetch_task(), so that get_next_submodule() is agnostic of how
to find submodules. This prepares for a subsequent commit will teach the
fetch machinery to also iterate through the list of changed
submodules (in addition to the index).
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:29 +0000 (16:14 -0800)]
submodule: store new submodule commits oid_array in a struct
This commit prepares for a future commit that will teach `git fetch
--recurse-submodules` how to fetch submodules that are present in
<gitdir>/modules, but are not populated. To do this, we need to store
more information about the changed submodule so that we can read the
submodule configuration from the superproject commit instead of the
filesystem.
Refactor the changed submodules string_list.util to hold a struct
instead of an oid_array. This struct only holds the new_commits
oid_array for now; more information will be added later.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:28 +0000 (16:14 -0800)]
submodule: inline submodule_commits() into caller
When collecting the string_list of changed submodule names, the new
submodules commits are stored in the string_list_item.util as an
oid_array. A subsequent commit will replace the oid_array with a struct
that has more information.
Prepare for this change by inlining submodule_commits() (which inserts
into the string_list and initializes the string_list_item.util) into its
only caller so that the code is easier to refactor later.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:27 +0000 (16:14 -0800)]
submodule: make static functions read submodules from commits
A future commit will teach "fetch --recurse-submodules" to fetch
unpopulated submodules. To prepare for this, teach the necessary static
functions how to read submodules from superproject commits using a
"treeish_name" argument (instead of always reading from the index and
filesystem) but do not actually change where submodules are read from.
Submodules will be read from commits when we fetch unpopulated
submodules.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:26 +0000 (16:14 -0800)]
t5526: create superproject commits with test helper
A few tests in t5526 use this pattern as part of their setup:
1. Create new commits in the upstream submodules (using
add_upstream_commit()).
2. In the upstream superprojects, add the new submodule commits from the
previous step.
A future commit will add more tests with this pattern, so reduce the
verbosity of present and future tests by introducing a test helper that
creates superproject commits. Since we now have two helpers that add
upstream commits, rename add_upstream_commit() to
add_submodule_commits().
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:25 +0000 (16:14 -0800)]
t5526: stop asserting on stderr literally
In the previous commit message, we noted that not all of the "git fetch"
stderr is relevant to the tests. Most of the test setup lines are
dedicated to these details of the stderr:
1. which repos (super/sub/deep) are involved in the fetch
2. the head of the remote-tracking branch before the fetch (i.e. $head1)
3. the head of the remote-tracking branch after the fetch (i.e. $head2)
1. and 3. are relevant because they tell us that the expected commit is
fetched by the expected repo, but 2. is completely irrelevant.
Stop asserting on $head1 by replacing it with a dummy value in the
actual and expected output. Do this by introducing test
helpers (write_expected_*()) that make it easier to construct the
expected output, and use sed to munge the actual output.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Tue, 8 Mar 2022 00:14:24 +0000 (16:14 -0800)]
t5526: introduce test helper to assert on fetches
Tests in t/t5526-fetch-submodules.sh are unnecessarily noisy:
* The tests have extra logic in order to reproduce the expected stderr
literally, but not all of these details (e.g. the head of the
remote-tracking branch before the fetch) are relevant to the test.
* The expect.err file is constructed by the add_upstream_commit() helper
as input into test_cmp, but most tests fetch a different combination
of repos from expect.err. This results in noisy tests that modify
parts of that expect.err to generate the expected output.
To address both of these issues, introduce a verify_fetch_result()
helper to t/t5526-fetch-submodules.sh that asserts on the output of "git
fetch --recurse-submodules" and handles the ordering of expect.err.
As a result, the tests no longer construct expect.err manually. Tests
still consider the old head of the remote-tracking branch ("$head1"),
but that will be fixed in a later commit.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change a fragile test pattern introduced in 65347030590 (Topo-sort
before --simplify-merges, 2008-08-03) to check the exit code of both
"git name-rev" and "git log".
This test as a whole would fail under SANITIZE=leak, but we'd pass
several "failing" tests due to hiding these exit codes before we'd
spot git dying with abort(). Now we'll instead spot all of the
failures.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change a fragile pattern introduced in 696acf45f96 (checkout:
implement "-" abbreviation, add docs and tests, 2009-01-17) to check
the exit code of both "git symbolic-ref" and "git rev-parse".
Without this change this test will become flaky e.g. under
SANITIZE=leak if some (but not all) memory leaks revealed by these
commands are fixed.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
apply tests: don't ignore "git ls-files" exit code, drop sub-shell
Fix code added in 969c877506c (git apply --directory broken for new
files, 2008-10-12) so that it doesn't invoke "git ls-files" on the
left-hand-side of a pipe, instead let's use an intermediate file.
Since we're doing that we can also drop the sub-shell that was here to
group the two.
There are a lot of these sorts of patterns in the test suite, and
there's no particular reason to fix this one other than in a preceding
commit all similar patterns except this one were fixed in
"t/t4128-apply-root.sh", so let's fix this one straggler as well.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Amend a prerequisite check added in 5c1ebcca4d1 (grep/icase: avoid
kwsset on literal non-ascii strings, 2016-06-25) to do invoke
'test-tool regex' in such a way that we'll notice if it dies under
SANITIZE=leak due to having a memory leak, as opposed to us not having
the "ICASE" support we're checking for.
Because we weren't making a distinction between the two I'd marked
these tests as passing under SANITIZE=leak in 03d85e21951 (leak tests:
mark remaining leak-free tests as such, 2021-12-17).
Doing this is tricky. Ideally "test_lazy_prereq" would materialize as
a "real" test that we could check the exit code of with the same
signal matching that "test_must_fail" does.
However lazy prerequisites aren't real tests, and are instead lazily
materialized in the guts of "test_have_prereq" when we've already
started another test.
We could detect the abort() (or similar) there and pass that exit code
down, and fail the test that caused the prerequisites to be
materialized.
But that would require extensive changes to test-lib.sh and
test-lib-functions.sh. Let's instead simply check if the exit code of
"test-tool regex" is zero, and if so set the prerequisites. If it's
non-zero let's run it again with "test_must_fail". We'll thus make a
distinction between "bad" non-zero (segv etc) and "good" (exit 1 etc.).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
rev-list tests: don't hide abort() in "test_expect_failure"
Change a couple of uses of "test_expect_failure" to use a
"test_expect_success" to positively assert the current behavior, and
replace the intent of "test_expect_failure" with a "TODO" comment int
the description.
As noted in [1] the "test_expect_failure" feature is overly eager to
accept any failure as OK, and thus by design hides segfaults, abort()
etc. Because of that I didn't notice in dd9cede9136 (leak tests: mark
some rev-list tests as passing with SANITIZE=leak, 2021-10-31) that
this test leaks memory under SANITIZE=leak.
I have some larger local changes to add a better
"test_expect_failure", which would work just like
"test_expect_success", but would allow us say "test_todo" here (and
"success" would emit a "not ok [...] # TODO", not "ok [...]".
So even though using "test_expect_success" here comes with its own
problems[2], let's use it as a narrow change to fix the problem at
hand here and stop conflating the current "success" with actual
SANITIZE=leak failures.
Change a fragile pattern introduced in 2b459b483cb (diff: make sure
work tree side is shown as 0{40} when different, 2008-03-02) to check
the exit code of "git rev-list", while we're at it let's get rid of
the needless sub-shell for invoking it in favor of the "-C" option.
Because of this I'd marked these tests as passing under SANITIZE=leak
in 16d4bd4f14e (leak tests: mark some diff tests as passing with
SANITIZE=leak, 2021-10-31), let's remove the
"TEST_PASSES_SANITIZE_LEAK=true" annotation as they no longer do.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change a fragile test pattern that's been with us ever since these
tests were introduced in [1], [2] and [3] to properly return the exit
code of the failing command on failure.
Because of this I'd marked this test as passing under SANITIZE=leak in
[4] and [5]. We need to remove those annotations as these tests will
no longer pass.
1. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
2. 0057c0917d3 (Add selftests verifying that we can parse notes trees
with various fanouts, 2009-10-09)
3. 048cdd4665e (t3305: Verify that adding many notes with git-notes
triggers increased fanout, 2010-02-13)
4. ca089724952 (leak tests: mark some notes tests as passing with
SANITIZE=leak, 2021-10-31)
5. 9081a421a6d (checkout: fix "branch info" memory leaks, 2021-11-16)
Amend a test added in 9c46c054ae4 (rev-parse: tests git rev-parse
--verify master@{n}, for various n, 2010-08-24) so that we'll stop
ignoring the exit code of "git reflog" by having it on the
left-hand-side of a pipe.
Because of this I'd marked this test as passing under SANITIZE=leak in f442c94638d (leak tests: mark some rev-parse tests as passing with
SANITIZE=leak, 2021-10-31). As all of it except this specific test
will now pass, let's skip it under the !SANITIZE_LEAK prerequisite.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge tests: use "test_must_fail" instead of ad-hoc pattern
As in the preceding commit change a similar fragile test pattern
introduced in b798671fa93 (merge-recursive: do not rudely die on
binary merge, 2007-08-14) to use a "test_must_fail" instead.
Before this we wouldn't distinguish normal "git merge" failures from
segfaults or abort(). Unlike the preceding commit we didn't end up
hiding any SANITIZE=leak failures in this case, but let's
correspondingly change these anyway.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
apply tests: use "test_must_fail" instead of ad-hoc pattern
Change a fragile test pattern introduced in 6b763c424e4 (git-apply: do
not read past the end of buffer, 2007-09-05). Before this we wouldn't
distinguish normal "git apply" failures from segfaults or abort().
I'd previously marked this test as passing under SANITIZE=leak in f54f48fc074 (leak tests: mark some apply tests as passing with
SANITIZE=leak, 2021-10-31). Let's remove that annotation as this test
will no longer pass.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.
Rather than use intermediate files let's rewrite these tests to a much
simpler but more exhaustive "test_tmp" where we'll ignore certain
fields in the output.
Note that this is not a faithful conversion of the previous
"read/test" in some cases, as we were ignoring more fields there than
we strictly needed to. Now we'll "test_cmp" everything we can, and
only ignore the likes of paths to $TEMPDIR etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix a test pattern that originated in f1af60bdba4 (Support 'diff=pgm'
attribute, 2007-04-22) so that we'll stop using "git diff" on the
left-hand-side of a pipe, and thus ignoring its exit code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
read-tree tests: check "diff-files" exit code on failure
Fix an issue with the exit code of "diff-files" being ignored, which
has been ignored ever since these tests were originally added in c859600954d ([PATCH] read-tree: save more user hassles during
fast-forward., 2005-06-07).
Since the exit code was ignored we'd hide errors here under
SANITIZE=leak, which resulted in me mistakenly marking these tests as
passing under SANITIZE=leak in e5a917fcf42 (unpack-trees: don't leak
memory in verify_clean_subdirectory(), 2021-10-07) and 4ea08416b8e (leak tests: mark a read-tree test as passing
SANITIZE=leak, 2021-10-31).
As it would be non-trivial to fix these tests (the leak is in
revision.c) let's un-mark them as passing under SANITIZE=leak in
addition to fixing the issue of ignoring the exit code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
tests: use "test_stdout_line_count", not "test $(git [...] | wc -l)"
Use the test_stdout_line_count helper added in cdff1bb5a3d (test-lib-functions: introduce test_stdout_line_count,
2021-07-04) so that we'll spot if git itself dies, segfaults etc in
these expressions.
Because we didn't distinguish these failure conditions before I'd
mistakenly marked these tests as passing under SANITIZE=leak in dd9cede9136 (leak tests: mark some rev-list tests as passing with
SANITIZE=leak, 2021-10-31).
While we're at it let's re-indent these lines to match our usual
style, as we're having to change all of them anyway.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
tests: change some 'test $(git) = "x"' to test_cmp
Change some of the patterns in the test suite where we were hiding the
exit code from "git" by invoking it in a sub-shell within a "test"
expression to use temporary files and test_cmp instead.
These are not all the occurrences of this anti-pattern, but these in
particular hid issues where LSAN was dying, and I'd thus marked these
tests as passing under the linux-leaks CI job in past commits with
"TEST_PASSES_SANITIZE_LEAK=true". Let's deal with that by either
removing that marking, or skipping specific tests under
!SANITIZE_LEAK.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hooks: fix an obscure TOCTOU "did we just run a hook?" race
Fix a Time-of-check to time-of-use (TOCTOU) race in code added in 680ee550d72 (commit: skip discarding the index if there is no
pre-commit hook, 2017-08-14).
This obscure race condition can occur if we e.g. ran the "pre-commit"
hook and it modified the index, but hook_exists() returns false later
on (e.g., because the hook itself went away, the directory became
unreadable, etc.). Then we won't call discard_cache() when we should
have.
The race condition itself probably doesn't matter, and users would
have been unlikely to run into it in practice. This problem has been
noted on-list when 680ee550d72 was discussed[1], but had not been
fixed.
This change is mainly intended to improve the readability of the code
involved, and to make reasoning about it more straightforward. It
wasn't as obvious what we were trying to do here, but by having an
"invoked_hook" it's clearer that e.g. our discard_cache() is happening
because of the earlier hook execution.
Let's also change this for the push-to-checkout hook. Now instead of
checking if the hook exists and either doing a push to checkout or a
push to deploy we'll always attempt a push to checkout. If the hook
doesn't exist we'll fall back on push to deploy. The same behavior as
before, without the TOCTOU race. See 0855331941b (receive-pack:
support push-to-checkout hook, 2014-12-01) for the introduction of the
previous behavior.
This leaves uses of hook_exists() in two places that matter. The
"reference-transaction" check in refs.c, see 67541597670 (refs:
implement reference transaction hook, 2020-06-19), and the
"prepare-commit-msg" hook, see 66618a50f9c (sequencer: run
'prepare-commit-msg' hook, 2018-01-24).
In both of those cases we're saving ourselves CPU time by not
preparing data for the hook that we'll then do nothing with if we
don't have the hook. So using this "invoked_hook" pattern doesn't make
sense in those cases.
The "reference-transaction" and "prepare-commit-msg" hook also aren't
racy. In those cases we'll skip the hook runs if we race with a new
hook being added, whereas in the TOCTOU races being fixed here we were
incorrectly skipping the required post-hook logic.
Fix a minor bug introduced in bc40ce4de61 (merge: --no-verify to
bypass pre-merge-commit hook, 2019-08-07), when that change made the
--no-verify option bypass the "pre-merge-commit" hook it didn't update
the corresponding find_hook() (later hook_exists()) condition.
As can be seen in the preceding commit in 6098817fd7f (git-merge:
honor pre-merge-commit hook, 2019-08-07) the two should go hand in
hand. There's no point in invoking discard_cache() here if the hook
couldn't have possibly updated the index.
It's buggy that we use "hook_exist()" here, and as discussed in the
subsequent commit it's subject to obscure race conditions that we're
about to fix, but for now this change is a strict improvement that
retains any caveats to do with the use of "hooks_exist()" as-is.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
string-list API: change "nr" and "alloc" to "size_t"
Change the "nr" and "alloc" members of "struct string_list" to use
"size_t" instead of "nr". On some platforms the size of an "unsigned
int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
bit unsigned. As "struct string_list" is a generic API we use in a lot
of places this might cause overflows.
As one example: code in "refs.c" keeps track of the number of refs
with a "size_t", and auxiliary code in builtin/remote.c in
get_ref_states() appends those to a "struct string_list".
While we're at it split the "nr" and "alloc" in string-list.h across
two lines, which is the case for most such struct member
declarations (e.g. in "strbuf.h" and "strvec.h").
Changing e.g. "int i" to "size_t i" in run_and_feed_hook() isn't
strictly necessary, and there are a lot more cases where we'll use a
local "int", "unsigned int" etc. variable derived from the "nr" in the
"struct string_list". But in that case as well as
add_wrapped_shortlog_msg() in builtin/shortlog.c we need to adjust the
printf format referring to "nr" anyway, so let's also change the other
variables referring to it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
gettext API users: don't explicitly cast ngettext()'s "n"
Change a few stray users of the inline gettext.h Q_() function to stop
casting its "n" argument, the vast majority of the users of that
wrapper API use the implicit cast to "unsigned long".
The ngettext() function (which Q_() resolves to) takes an "unsigned
long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
add stub Q_() wrapper for ngettext, 2011-03-09). The function isn't
ours, but provided by e.g. GNU libintl.
This amends code added in added in 7171a0b0cf5 (index-pack: correct
"len" type in unpack_data(), 2016-07-13). The cast it added for the
printf format to die() was needed, but not the cast to Q_().
Likewise the casts in strbuf.c added in 8f354a1faed (l10n: localizable
upload progress messages, 2019-07-02) and for
builtin/merge-recursive.c in ccf7813139f (i18n: merge-recursive: mark
error messages for translation, 2016-09-15) weren't needed.
In the latter case the cast was copy/pasted from the argument to
warning() itself, added in b74d779bd90 (MinGW: Fix compiler warning in
merge-recursive, 2009-05-23). The cast for warning() is needed, but
not the one for ngettext()'s "n" argument.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Derrick Stolee [Wed, 2 Mar 2022 14:45:13 +0000 (09:45 -0500)]
commit-graph: declare bankruptcy on GDAT chunks
The Generation Data (GDAT) and Generation Data Overflow (GDOV) chunks
store corrected commit date offsets, used for generation number v2.
Recent changes have demonstrated that previous versions of Git were
incorrectly parsing data from these chunks, but might have also been
writing them incorrectly.
I asserted [1] that the previous fixes were sufficient because the known
reasons for incorrectly writing generation number v2 data relied on
parsing the information incorrectly out of a commit-graph file, but the
previous versions of Git were not reading the generation number v2 data.
However, Patrick demonstrated [2] a case where in split commit-graphs
across an alternate boundary (and possibly some other special
conditions) it was possible to have a commit-graph that was generated by
a previous version of Git have incorrect generation number v2 data which
results in errors like the following:
Clearly, there is something else going on. The situation is not
completely understood, but the errors do not reproduce if the
commit-graphs are all generated by a Git version including these recent
fixes.
If we cannot trust the existing data in the GDAT and GDOV chunks, then
we can alter the format to change the chunk IDs for these chunks. This
causes the new version of Git to silently ignore the older chunks (and
disabling generation number v2 in the process) while writing new
commit-graph files with correct data in the GDA2 and GDO2 chunks.
Update commit-graph-format.txt including a historical note about these
deprecated chunks.
Reported-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 7 Mar 2022 05:25:32 +0000 (21:25 -0800)]
Merge branch 'ab/c99-designated-initializers'
Use designated initializers we started using in mid 2017 in more
parts of the codebase that are relatively quiescent.
* ab/c99-designated-initializers:
fast-import.c: use designated initializers for "partial" struct assignments
refspec.c: use designated initializers for "struct refspec_item"
convert.c: use designated initializers for "struct stream_filter*"
userdiff.c: use designated initializers for "struct userdiff_driver"
archive-*.c: use designated initializers for "struct archiver"
object-file: use designated initializers for "struct git_hash_algo"
trace2: use designated initializers for "struct tr2_dst"
trace2: use designated initializers for "struct tr2_tgt"
imap-send.c: use designated initializers for "struct imap_server_conf"
Junio C Hamano [Mon, 7 Mar 2022 05:25:31 +0000 (21:25 -0800)]
Merge branch 'ds/worktree-docs'
Tighten the language around "working tree" and "worktree" in the
docs.
* ds/worktree-docs:
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: use 'worktree' over 'working tree'
worktree: extract checkout_worktree()
worktree: extract copy_sparse_checkout()
worktree: extract copy_filtered_worktree_config()
worktree: combine two translatable messages
Junio C Hamano [Mon, 7 Mar 2022 05:25:30 +0000 (21:25 -0800)]
Merge branch 'rs/bisect-executable-not-found'
A not-so-common mistake is to write a script to feed "git bisect
run" without making it executable, in which case all tests will
exit with 126 or 127 error codes, even on revisions that are marked
as good. Try to recognize this situation and stop iteration early.
* rs/bisect-executable-not-found:
bisect--helper: double-check run command on exit code 126 and 127
bisect: document run behavior with exit codes 126 and 127
bisect--helper: release strbuf and strvec on run error
bisect--helper: report actual bisect_state() argument on error
Junio C Hamano [Mon, 7 Mar 2022 05:25:30 +0000 (21:25 -0800)]
Merge branch 'en/sparse-checkout-fixes'
Further polishing of "git sparse-checkout".
* en/sparse-checkout-fixes:
sparse-checkout: reject arguments in cone-mode that look like patterns
sparse-checkout: error or warn when given individual files
sparse-checkout: pay attention to prefix for {set, add}
sparse-checkout: correctly set non-cone mode when expected
sparse-checkout: correct reapply's handling of options
Junio C Hamano [Mon, 7 Mar 2022 05:25:30 +0000 (21:25 -0800)]
Merge branch 'cg/t3903-modernize'
Test modernization.
* cg/t3903-modernize:
tests: make the code more readable
tests: allow testing if a path is truly a file or a directory
t/t3903-stash.sh: replace test [-d|-f] with test_path_is_*
Glen Choo [Sat, 5 Mar 2022 00:14:01 +0000 (16:14 -0800)]
submodule--helper update-clone: check for --filter and --init
"git submodule update --filter" also requires the "--init" option. Teach
update-clone to do this usage check in C and remove the check from
git-submodule.sh.
In addition, change update-clone's usage string so that it teaches users
about "git submodule update" instead of "git submodule--helper
update-clone" (the string is copied from git-submodule.sh). This should
be more helpful to users since they don't invoke update-clone directly.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 5 Mar 2022 00:13:59 +0000 (16:13 -0800)]
submodule--helper: remove ensure-core-worktree
Move the logic of "git submodule--helper ensure-core-worktree" into
run-update-procedure, and since this makes the ensure-core-worktree
command obsolete, remove it.
As a result, the order of two operations in git-submodule.sh is
reversed: 'set the value of core.worktree' now happens after the call to
"git submodule--helper relative-path". This is safe - "relative-path"
does not depend on the value of core.worktree.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 5 Mar 2022 00:13:58 +0000 (16:13 -0800)]
submodule--helper update-clone: learn --init
Teach "git submodule--helper update-clone" the --init flag and remove
the corresponding shell code.
When the `--init` flag is passed to the subcommand, we do not spawn a
new subprocess and call `submodule--helper init` on the submodule paths,
because the Git machinery is not able to pick up the configuration
changes introduced by that init call. So we instead run the
`init_submodule_cb()` callback over each submodule in the same process.
Atharva Raykar [Sat, 5 Mar 2022 00:13:57 +0000 (16:13 -0800)]
submodule--helper: allow setting superprefix for init_submodule()
We allow callers of the `init_submodule()` function to optionally
override the superprefix from the environment.
We need to enable this option because in our conversion of the update
command that will follow, the '--init' option will be handled through
this API. We will need to change the superprefix at that time to ensure
the display paths show correctly in the output messages.
Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We create a function called `do_get_submodule_displaypath()` that
generates the display path required by several submodule functions, and
takes a custom superprefix parameter, instead of reading it from the
environment.
We then redefine the existing `get_submodule_displaypath()` function
as a call to this new function, where the superprefix is obtained from
the environment.
Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach run-update-procedure to handle --remote instead of parsing
--remote in git-submodule.sh. As a result, "git submodule--helper
[print-default-remote|remote-branch]" have no more callers, so remove
them.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
submodule--helper: don't use bitfield indirection for parse_options()
Do away with the indirection of local variables added in c51f8f94e5b (submodule--helper: run update procedures from C,
2021-08-24).
These were only needed because in C you can't get a pointer to a
single bit, so we were using intermediate variables instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Atharva Raykar [Sat, 5 Mar 2022 00:13:53 +0000 (16:13 -0800)]
submodule--helper: get remote names from any repository
`get_default_remote()` retrieves the name of a remote by resolving the
refs from of the current repository's ref store.
Thus in order to use it for retrieving the remote name of a submodule,
we have to start a new subprocess which runs from the submodule
directory.
Let's instead introduce a function called `repo_get_default_remote()`
which takes any repository object and retrieves the remote accordingly.
`get_default_remote()` is then defined as a call to
`repo_get_default_remote()` with 'the_repository' passed to it.
Now that we have `repo_get_default_remote()`, we no longer have to start
a subprocess that called `submodule--helper get-default-remote` from
within the submodule directory.
So let's make a function called `get_default_remote_submodule()` which
takes a submodule path, and returns the default remote for that
submodule, all within the same process.
We can now use this function to save an unnecessary subprocess spawn in
`sync_submodule()`, and also in a subsequent patch, which will require
this functionality.
Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> Helped-by: Glen Choo <chooglen@google.com> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Sat, 5 Mar 2022 00:13:51 +0000 (16:13 -0800)]
submodule--helper: reorganize code for sh to C conversion
Introduce a function, update_submodule2(), that will implement the
functionality of run-update-procedure and its surrounding shell code in
submodule.sh. This name is temporary; it will replace update_submodule()
when the sh to C conversion is complete.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
submodule tests: test for init and update failure output
Amend some submodule tests to test for the failure output of "git
submodule [update|init]". The lack of such tests hid a regression in
an earlier version of a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
repository.c: free the "path cache" in repo_clear()
The "struct path_cache" added in 102de880d24 (path.c: migrate global
git_path_* to take a repository argument, 2018-05-17) is only used
directly by code in repository.[ch] (but populated in path.[ch]).
Let's move this code to repository.[ch], and stop leaking this memory
when we run repo_clear(). To avoid the cast change it from a "const
char *" to a "char *".
This also removes the "PATH_CACHE_INIT" macro, which has never been
used for anything. For the "struct repository" we already make a hard
assumption that it (and "the_repository") can be identically
initialized by making it a "static" variable, so making use of a
"PATH_CACHE_INIT" somewhere would have been confusing.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Amend code added in d9c66f0b5bf (range-diff: first rudimentary
implementation, 2018-08-13) to use a "goto cleanup" pattern. This
makes for less code, and frees memory that we'd previously leak.
The reason for changing free(util) to FREE_AND_NULL(util) is because
at the end of the function we append the contents of "util" to a
"struct string_list" if it's non-NULL.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Create a public release_patch() version of the private free_patch()
function added in 13b5af22f39 (apply: move libified code from
builtin/apply.c to apply.{c,h}, 2016-04-22). Unlike the existing
function this one doesn't free() the "struct patch" itself, so we can
use it for variables on the stack.
Use it in range-diff.c to fix a memory leak in common range-diff
invocations, e.g.:
lockfile API users: simplify and don't leak "path"
Fix a memory leak in code added in 6c622f9f0bb (commit-graph: write
commit-graph chains, 2019-06-18). We needed to free the "lock_name" if
we encounter errors, and the "graph_name" after we'd run unlink() on
it.
For the case of write_commit_graph_file() refactoring the code to free
the "lock_name" after we were done using the "struct lock_file lk"
would have made the control flow more complex. Luckily we can free the
"lock_file" right after the hold_lock_file_for_update() call, if it
makes use of "path" at all it'll have copied its contents to a "struct
strbuf" of its own.
While I'm at it let's fix code added in fb10ca5b543 (sparse-checkout:
write using lockfile, 2019-11-21) in write_patterns_and_update() to
avoid the same complexity that I thought I needed when I wrote the
initial fix for write_commit_graph_file(). We can free the
"sparse_filename" right after calling hold_lock_file_for_update(), we
don't need to wait until we're exiting the function to do so.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
commit-graph: stop fill_oids_from_packs() progress on error and free()
Fix a bug in fill_oids_from_packs(), we should always stop_progress(),
but did not do so if we returned an error here. This also plugs a
memory leak in those cases by releasing the two "struct strbuf"
variables the function uses.
While I'm at it stop hardcoding "-1" here and just use the return
value of error() instead, which happens to be "-1".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
commit-graph: fix memory leak in misused string_list API
When this code was migrated to the string_list API in d88b14b3fd6 (commit-graph: use string-list API for input, 2018-06-27)
it was made to use used both STRING_LIST_INIT_NODUP and a
strbuf_detach() pattern.
Those should not be used together if string_list_clear() is expected
to free the memory, instead we need to either use STRING_LIST_INIT_DUP
with a string_list_append_nodup(), or a STRING_LIST_INIT_NODUP and
manually fiddle with the "strdup_strings" member before calling
string_list_clear(). Let's do the former.
Since "strdup_strings = 1" is set now other code might be broken by
relying on "pack_indexes" not to duplicate it strings, but that
doesn't happen. When we pass this down to write_commit_graph() that
code uses the "struct string_list" without modifying it. Let's add a
"const" to the variable to have the compiler enforce that assumption.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
submodule--helper: fix trivial leak in module_add()
Fix a memory leak in code added in a6226fd772b (submodule--helper:
convert the bulk of cmd_add() to C, 2021-08-10). If "realrepo" isn't a
copy of the "repo" member we should free() it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Amend the logic added in fddf2ebe388 (transport: teach all vtables to
allow fetch first, 2019-08-21) and save ourselves pointless work in
fetch_refs_from_bundle().
The fetch_refs_from_bundle() caller doesn't care about the "struct
ref *result" return value of get_refs_from_bundle(), and doesn't need
any of the work we were doing in looping over the
"data->header.references" in get_refs_from_bundle().
So this change saves us work, and also fixes a memory leak that we had
when called from fetch_refs_from_bundle(). The other caller of
get_refs_from_bundle() is the "get_refs_list" member we set up for the
"struct transport_vtable bundle_vtable". That caller does care about
the "struct ref *result" return value.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Plug a memory leak in credential_apply_config() by adding and using a
new urlmatch_config_release() function. This just does a
string_list_clear() on the "vars" member.
This finished up work on normalizing the init/free pattern in this
API, started in 73ee449bbf2 (urlmatch.[ch]: add and use
URLMATCH_CONFIG_INIT, 2021-10-01).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Amend the freeing logic added in e6e045f8031 (diff.c: buffer all
output if asked to, 2017-06-29) to free the containing "buf" in
addition to its members.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
merge-base: free() allocated "struct commit **" list
Fix a memory leak in 53eda89b2fe (merge-base: teach "git merge-base"
to drive underlying merge_bases_many(), 2008-07-30) by calling free()
on the "struct commit **" list used by "git merge-base".
This gets e.g. "t6010-merge-base.sh" closer to passing under
SANITIZE=leak, it failed 8 tests before when compiled with that
option, and now fails only 5 tests.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix various memory leaks in "git index-pack", due to how tightly
coupled this command is with the revision walking this doesn't make
any new tests pass.
But e.g. this now passes, and had several failures before, i.e. we
still have failures in tests 3, 5 etc., which are being skipped here.
./t5300-pack-object.sh --run=1-2,4,6-27,30-42
It is a bit odd that we'll free "opts.anomaly", since the "opts" is a
"struct pack_idx_option" declared in pack.h. In pack-write.c there's a
reset_pack_idx_option(), but it only wipes the contents, but doesn't
free() anything.
Doing this here in cmd_index_pack() is correct because while the
struct is declared in pack.h, this code in builtin/index-pack.c (in
read_v2_anomalous_offsets()) is what allocates the "opts.anomaly", so
we should also free it here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elia Pinto [Fri, 4 Mar 2022 13:37:02 +0000 (13:37 +0000)]
test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34
In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment
variables have been replaced by GLIBC_TUNABLES. Also the new
glibc requires that you preload a library called libc_malloc_debug.so
to get these features.
Using the ordinary glibc system variable detect if this is glibc >= 2.34 and
use GLIBC_TUNABLES and the new library.
This patch was inspired by a Richard W.M. Jones ndbkit patch
Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Todd Zullinger [Fri, 4 Mar 2022 10:25:19 +0000 (11:25 +0100)]
t/lib-gpg: kill all gpg components, not just gpg-agent
The gpg-agent is one of several processes that newer releases of GnuPG
start automatically. Issue a kill to each of them to ensure they do not
affect separate tests. (Yes, the separate GNUPGHOME should do that
already. If we find that is case, we could drop the --kill entirely.)
In terms of compatibility, the 'all' keyword was added to the --kill &
--reload options in GnuPG 2.1.18. Debian and RHEL are often used as
indicators of how a change might affect older systems we often try to
support.
- Debian Strech (old old stable), which has limited security support
until June 2022, has GnuPG 2.1.18 (or 2.2.x in backports).
- CentOS/RHEL 7, which is supported until June 2024, has GnuPG
2.0.22, which lacks the --kill option, so the change won't have
any impact.
Signed-off-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Todd Zullinger [Fri, 4 Mar 2022 10:25:18 +0000 (11:25 +0100)]
t/lib-gpg: reload gpg components after updating trustlist
With gpgsm from gnupg-2.3, the changes to the trustlist.txt do not
appear to be picked up without refreshing the gpg-agent. Use the 'all'
keyword to reload all of the gpg components. The scdaemon is started as
a child of gpg-agent, for example.
We used to have a --kill at this spot, but I removed it in 2e285e7803
(t/lib-gpg: drop redundant killing of gpg-agent, 2019-02-07). It seems
like it might be necessary (again) for 2.3.
Signed-off-by: Todd Zullinger <tmz@pobox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fabian Stelzer [Fri, 4 Mar 2022 10:25:17 +0000 (11:25 +0100)]
gpg-interface/gpgsm: fix for v2.3
Checking if signing was successful will now accept '[GNUPG]:
SIG_CREATED' on the beginning of the first or any subsequent line. Not
just explictly the second one anymore.
Gpgsm v2.3 changed its output when listing keys from `fingerprint` to
`sha1/2 fpr`. This leads to the gpgsm tests silently not being executed
because of a failed prerequisite.
Switch to gpg's `--with-colons` output format when evaluating test
prerequisites to make parsing more robust. This also allows us to
combine the existing grep/cut/tr/echo pipe for writing the trustlist.txt
into a single awk expression.
Adjust error message checking in test for v2.3 specific output changes.
Helped-By: Junio C Hamano <gitster@pobox.com> Helped-By: Todd Zullinger <tmz@pobox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
log tests: fix "abort tests early" regression in ff37a60c369
Fix a regression in ff37a60c369 (log tests: check if grep_config() is
called by "log"-like cmds, 2022-02-16), a "test_done" command used
during development made it into a submitted patch causing tests 41-136
in t/t4202-log.sh to be skipped.
Reported-by: Fabian Stelzer <fs@gigacodes.de> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Thu, 3 Mar 2022 22:25:18 +0000 (17:25 -0500)]
builtin/remote.c: show progress when renaming remote references
When renaming a remote, Git needs to rename all remote tracking
references to the remote's new name (e.g., renaming
"refs/remotes/old/foo" to "refs/remotes/new/foo" when renaming a remote
from "old" to "new").
This can be somewhat slow when there are many references to rename,
since each rename is done in a separate call to rename_ref() as opposed
to grouping all renames together into the same transaction. It would be
nice to execute all renames as a single transaction, but there is a
snag: the reference transaction backend doesn't support renames during a
transaction (only individually, via rename_ref()).
The reasons there are described in more detail in [1], but the main
problem is that in order to preserve the existing reflog, it must be
moved while holding both locks (i.e., on "oldname" and "newname"), and
the ref transaction code doesn't support inserting arbitrary actions
into the middle of a transaction like that.
As an aside, adding support for this to the ref transaction code is
less straightforward than inserting both a ref_update() and ref_delete()
call into the same transaction. rename_ref()'s special handling to
detect D/F conflicts would need to be rewritten for the transaction code
if we wanted to proactively catch D/F conflicts when renaming a
reference during a transaction. The reftable backend could support this
much more readily because of its lack of D/F conflicts.
Instead of a more complex modification to the ref transaction code,
display a progress meter when running verbosely in order to convince the
user that Git is doing work while renaming a remote.
This is mostly done as-expected, with the minor caveat that we
intentionally count symrefs renames twice, since renaming a symref takes
place over two separate calls (one to delete the old one, and another to
create the new one).
Suggested-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Taylor Blau [Thu, 3 Mar 2022 22:25:16 +0000 (17:25 -0500)]
builtin/remote.c: parse options in 'rename'
The 'git remote rename' command doesn't currently take any command-line
arguments besides the existing and new name of a remote, and so has no
need to call parse_options().
But the subsequent patch will add a `--[no-]progress` option, in which
case we will need to call parse_options().
Do so now so as to avoid cluttering the following patch with noise, like
adjusting setting `rename.{old,new}_name` to argv[0] and argv[1], since
parse_options handles advancing argv past the name of the sub-command.
Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t5503: simplify setup of test which exercises failure of backfill
In the testcase to exercise backfilling of tags for fetches we evoke a
failure of the backfilling mechanism by creating a reference that later
on causes a D/F conflict. Because the assumption was that git-fetch(1)
would notice the D/F conflict early on this conflicting reference was
created via the reference-transaction hook just when we were about to
write the backfilled tag. As it turns out though this is not the case,
and the fetch fails in the same way when we create the conflicting ref
up front.
Simplify the test setup creating the reference up front, which allows us
to get rid of the hook script.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Makefiles: add and use wildcard "mkdir -p" template
Add a template to do the "mkdir -p" of $(@D) (the parent dir of $@)
for us, and use it for the "make lint-docs" targets I added in 8650c6298c1 (doc lint: make "lint-docs" non-.PHONY, 2021-10-15).
As seen in 4c64fb5aad9 (Documentation/Makefile: fix lint-docs mkdir
dependency, 2021-10-26) maintaining these manual lists of parent
directory dependencies is fragile, in addition to being obviously
verbose.
I used this pattern at the time because I couldn't find another method
than "order-only" prerequisites to avoid doing a "mkdir -p $(@D)" for
every file being created, which as noted in [1] would be significantly
slower.
But as it turns out we can use this neat trick of only doing a "mkdir
-p" if the $(wildcard) macro tells us the path doesn't exist. A re-run
of a performance test similar to that noted downthread of [1] in [2]
shows that this is faster, in addition to being less verbose and more
reliable (this uses my "git-hyperfine" thin wrapper for "hyperfine"[3]):
$ git -c hyperfine.hook.setup= hyperfine -L rev HEAD~1,HEAD~0 -s 'make -C Documentation lint-docs' -p 'rm -rf Documentation/.build' 'make -C Documentation -j1 lint-docs'
Benchmark 1: make -C Documentation -j1 lint-docs' in 'HEAD~1
Time (mean ± σ): 2.914 s ± 0.062 s [User: 2.449 s, System: 0.489 s]
Range (min … max): 2.834 s … 3.020 s 10 runs
Benchmark 2: make -C Documentation -j1 lint-docs' in 'HEAD~0
Time (mean ± σ): 2.315 s ± 0.062 s [User: 1.950 s, System: 0.386 s]
Range (min … max): 2.229 s … 2.397 s 10 runs
Summary
'make -C Documentation -j1 lint-docs' in 'HEAD~0' ran
1.26 ± 0.04 times faster than 'make -C Documentation -j1 lint-docs' in 'HEAD~1'
So let's use that pattern both for the "lint-docs" target, and a few
miscellaneous other targets.
This method of creating parent directories is explicitly racy in that
we don't know if we're going to say always create a "foo" followed by
a "foo/bar" under parallelism, or skip the "foo" because we created
"foo/bar" first. In this case it doesn't matter for anything except
that we aren't guaranteed to get the same number of rules firing when
running make in parallel.
Makefile: add "$(QUIET)" boilerplate to shared.mak
The $(QUIET) variables we define are largely duplicated between our
various Makefiles, let's define them in the new "shared.mak" instead.
Since we're not using the environment to pass these around we don't
need to export the "QUIET_GEN" and "QUIET_BUILT_IN" variables
anymore. The "QUIET_GEN" variable is used in "git-gui/Makefile" and
"gitweb/Makefile", but they've got their own definition for those. The
"QUIET_BUILT_IN" variable is only used in the top-level "Makefile". We
still need to export the "V" variable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Makefile: move $(comma), $(empty) and $(space) to shared.mak
Move these variables over to the shared.mak, we'll make use of them in
a subsequent commit.
Note that there's reason for these to be "simply expanded variables",
i.e. to use ":=" assignments instead of lazily expanded "="
assignments. We could use "=", but let's leave this as-is for now for
ease of review.
See 425ca6710b2 (Makefile: allow combining UBSan with other
sanitizers, 2017-07-15) for the commit that introduced these.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This was added in 30248886ce8 (Makefile: disable default implicit
rules, 2010-01-26), let's move it to the top of "shared.mak" so it'll
apply to all our Makefiles.
This doesn't benefit the main Makefile at all, since it already had
the rule, but since we're including shared.mak in other Makefiles
starts to benefit them. E.g. running the 'man" target is now faster:
$ git -c hyperfine.hook.setup= hyperfine -L rev HEAD~1,HEAD~0 -s 'make -C Documentation man' 'make -C Documentation -j1 man'
Benchmark 1: make -C Documentation -j1 man' in 'HEAD~1
Time (mean ± σ): 121.7 ms ± 8.8 ms [User: 105.8 ms, System: 18.6 ms]
Range (min … max): 112.8 ms … 148.4 ms 26 runs
Benchmark 2: make -C Documentation -j1 man' in 'HEAD~0
Time (mean ± σ): 97.5 ms ± 8.0 ms [User: 80.1 ms, System: 20.1 ms]
Range (min … max): 89.8 ms … 111.8 ms 32 runs
Summary
'make -C Documentation -j1 man' in 'HEAD~0' ran
1.25 ± 0.14 times faster than 'make -C Documentation -j1 man' in 'HEAD~1'
The reason for that can be seen when comparing that run with
"--debug=a". Without this change making a target like "git-status.1"
will cause "make" to consider not only "git-status.txt", but
"git-status.txt.o", as well as numerous other implicit suffixes such
as ".c", ".cc", ".cpp" etc. See [1] for a more detailed before/after
example.
So this is causing us to omit a bunch of work we didn't need to
do. For making "git-status.1" the "--debug=a" output is reduced from
~140k lines to ~6k.
Makefile: define $(LIB_H) in terms of $(FIND_SOURCE_FILES)
Combine the definitions of $(FIND_SOURCE_FILES) and $(LIB_H) to speed
up the Makefile, as these are the two main expensive $(shell) commands
that we execute unconditionally.
When see what was in $(FOUND_SOURCE_FILES) that wasn't in $(LIB_H) via
the ad-hoc test of:
I.e. we only had a discrepancy when it came to
t/helper/test-tool.h. In terms of correctness this was broken before,
but now works:
$ make t/helper/test-tool.hco
HDR t/helper/test-tool.h
This speeds things up a lot:
$ git -c hyperfine.hook.setup= hyperfine -L rev HEAD~1,HEAD~0 -s 'make NO_TCLTK=Y' 'make -j1 NO_TCLTK=Y' --warmup 10 -M 10
Benchmark 1: make -j1 NO_TCLTK=Y' in 'HEAD~1
Time (mean ± σ): 159.9 ms ± 6.8 ms [User: 137.2 ms, System: 28.0 ms]
Range (min … max): 154.6 ms … 175.9 ms 10 runs
Benchmark 2: make -j1 NO_TCLTK=Y' in 'HEAD~0
Time (mean ± σ): 100.0 ms ± 1.3 ms [User: 84.2 ms, System: 20.2 ms]
Range (min … max): 98.8 ms … 102.8 ms 10 runs
Summary
'make -j1 NO_TCLTK=Y' in 'HEAD~0' ran
1.60 ± 0.07 times faster than 'make -j1 NO_TCLTK=Y' in 'HEAD~1'
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Makefile: disable GNU make built-in wildcard rules
Override built-in rules of GNU make that use a wildcard target. This
can speeds things up significantly as we don't need to stat() so many
files. GNU make does that by default to see if it can retrieve their
contents from RCS or SCCS. See [1] for an old mailing list discussion
about how to disable these.
The speed-up may vary. I've seen 1-10% depending on the speed of the
local disk, caches, -jN etc. Running:
strace -f -c -S calls make -j1 NO_TCLTK=Y
Shows that we reduce the number of syscalls we make, mostly in "stat"
calls.
We could also invoke make with "-r" by setting "MAKEFLAGS = -r"
early. Doing so might make us a bit faster still. But doing so is a
much bigger hammer, since it will disable all built-in rules,
some (all?) of which can be seen with:
make -f/dev/null -p | grep -v -e ^# -e ^$
We may have something that relies on them, so let's go for the more
isolated optimization here that gives us most or all of the wins.
Makefiles: add "shared.mak", move ".DELETE_ON_ERROR" to it
We have various behavior that's shared across our Makefiles, or that
really should be (e.g. via defined templates). Let's create a
top-level "shared.mak" to house those sorts of things, and start by
adding the ".DELETE_ON_ERROR" flag to it.
See my own 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR"
flag, 2021-06-29) and db10fc6c09f (doc: simplify Makefile using
.DELETE_ON_ERROR, 2021-05-21) for the addition and use of the
".DELETE_ON_ERROR" flag.
I.e. this changes the behavior of existing rules in the altered
Makefiles (except "Makefile" & "Documentation/Makefile"). I'm
confident that this is safe having read the relevant rules in those
Makfiles, and as the GNU make manual notes that it isn't the default
behavior is out of an abundance of backwards compatibility
caution. From edition 0.75 of its manual, covering GNU make 4.3:
[Enabling '.DELETE_ON_ERROR' is] almost always what you want
'make' to do, but it is not historical practice; so for
compatibility, you must explicitly request it.
This doesn't introduce a bug by e.g. having this
".DELETE_ON_ERROR" flag only apply to this new shared.mak, Makefiles
have no such scoping semantics.
It does increase the danger that any Makefile without an explicit "The
default target of this Makefile is..." snippet to define the default
target as "all" could have its default rule changed if our new
shared.mak ever defines a "real" rule. In subsequent commits we'll be
careful not to do that, and such breakage would be obvious e.g. in the
case of "make -C t".
We might want to make that less fragile still (e.g. by using
".DEFAULT_GOAL" as noted in the preceding commit), but for now let's
simply include "shared.mak" without adding that boilerplate to all the
Makefiles that don't have it already. Most of those are already
exposed to that potential caveat e.g. due to including "config.mak*".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
scalar Makefile: use "The default target of..." pattern
Make the "contrib/scalar/Makefile" be stylistically consistent with
the top-level "Makefile" in first declaring "all" to be the default
rule, followed by including other Makefile snippets.
This adjusts code added in 0a43fb22026 (scalar: create a rudimentary
executable, 2021-12-03), it further ensures that when we add another
"include" file in a subsequent commit that the included file won't be
the one to define our default target.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 3 Mar 2022 21:58:39 +0000 (13:58 -0800)]
am/apply: warn if we end up reading patches from terminal
In an interactive session, "git am" without arguments, or even
worse, "git am --whitespace file", waits silently for the user to
feed the patches from the standard input (presumably by typing or
copy-pasting). Give a feedback message to the user when this
happens, as it is unlikely that the user meant to do so.
John Cai [Wed, 2 Mar 2022 22:27:24 +0000 (22:27 +0000)]
stash: call reflog_delete() in reflog.c
Now that cmd_reflog_delete has been libified an exported it into a new
reflog.c library so we can call it directly from builtin/stash.c. This
not only gives us a performance gain since we don't need to create a
subprocess, but it also allows us to use the ref transactions api in the
future.
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
John Cai [Wed, 2 Mar 2022 22:27:23 +0000 (22:27 +0000)]
reflog: libify delete reflog function and helpers
Currently stash shells out to reflog in order to delete refs. In an
effort to reduce how much we shell out to a subprocess, libify the
functionality that stash needs into reflog.c.
Add a reflog_delete function that is pretty much the logic in the while
loop in builtin/reflog.c cmd_reflog_delete(). This is a function that
builtin/reflog.c and builtin/stash.c can both call.
Also move functions needed by reflog_delete and export them.
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
John Cai [Wed, 2 Mar 2022 22:27:22 +0000 (22:27 +0000)]
stash: add tests to ensure reflog --rewrite --updatref behavior
There is missing test coverage to ensure that the resulting reflogs
after a git stash drop has had its old oid rewritten if applicable, and
if the refs/stash has been updated if applicable.
Add two tests that verify both of these happen.
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Sat, 26 Feb 2022 06:12:22 +0000 (22:12 -0800)]
repo_read_index: add config to expect files outside sparse patterns
Typically with sparse checkouts, we expect files outside the sparsity
patterns to be marked as SKIP_WORKTREE and be missing from the working
tree. Sometimes this expectation would be violated however; including
in cases such as:
* users grabbing files from elsewhere and writing them to the worktree
(perhaps by editing a cached copy in an editor, copying/renaming, or
even untarring)
* various git commands having incomplete or no support for the
SKIP_WORKTREE bit[1,2]
* users attempting to "abort" a sparse-checkout operation with a
not-so-early Ctrl+C (updating $GIT_DIR/info/sparse-checkout and the
working tree is not atomic)[3].
When the SKIP_WORKTREE bit in the index did not reflect the presence of
the file in the working tree, it traditionally caused confusion and was
difficult to detect and recover from. So, in a sparse checkout, since af6a51875a (repo_read_index: clear SKIP_WORKTREE bit from files present
in worktree, 2022-01-14), Git automatically clears the SKIP_WORKTREE
bit at index read time for entries corresponding to files that are
present in the working tree.
There is another workflow, however, where it is expected that paths
outside the sparsity patterns appear to exist in the working tree and
that they do not lose the SKIP_WORKTREE bit, at least until they get
modified. A Git-aware virtual file system[4] takes advantage of its
position as a file system driver to expose all files in the working
tree, fetch them on demand using partial clone on access, and tell Git
to pay attention to them on demand by updating the sparse checkout
pattern on writes. This means that commands like "git status" only have
to examine files that have potentially been modified, whereas commands
like "ls" are able to show the entire codebase without requiring manual
updates to the sparse checkout pattern.
Thus since af6a51875a, Git with such Git-aware virtual file systems
unsets the SKIP_WORKTREE bit for all files and commands like "git
status" have to fetch and examine them all.
Introduce a configuration setting sparse.expectFilesOutsideOfPatterns to
allow limiting the tracked set of files to a small set once again. A
Git-aware virtual file system or other application that wants to
maintain files outside of the sparse checkout can set this in a
repository to instruct Git not to check for the presence of
SKIP_WORKTREE files. The setting defaults to false, so most users of
sparse checkout will still get the benefit of an automatically updating
index to recover from the variety of difficult issues detailed in af6a51875a for paths with SKIP_WORKTREE set despite the path being
present.
[1] https://lore.kernel.org/git/xmqqbmb1a7ga.fsf@gitster-ct.c.googlers.com/
[2] The three long paragraphs in the middle of
https://lore.kernel.org/git/CABPp-BH9tju7WVm=QZDOvaMDdZbpNXrVWQdN-jmfN8wC6YVhmw@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BFnFpzwGC11TLoLs8YK5yiisA5D5-fFjXnJsbESVDwZsA@mail.gmail.com/
[4] such as the vfsd described in
https://lore.kernel.org/git/20220207190320.2960362-1-jonathantanmy@google.com/
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Wed, 2 Mar 2022 04:19:21 +0000 (04:19 +0000)]
merge-ort: exclude messages from inner merges by default
merge-recursive would only report messages from inner merges when the
GIT_MERGE_VERBOSITY was set to 5. Do the same for merge-ort.
Note that somewhat reverts 0d83d8240d ("merge-ort: mark conflict/warning
messages from inner merges as omittable", 2022-02-02) based on two
facts:
* This commit basically removes the showing of messages from inner
merges as well, at least by default. The only difference is that
users can request to get them back by turning up the verbosity.
* Messages from inner merges are specially annotated since 4a3d86e1bb
("merge-ort: make informational messages from recursive merges
clearer", 2022-02-17). The ability to distinguish them from outer
merge comments make them less problematic to include, and easier
for humans to parse.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Glen Choo [Wed, 2 Mar 2022 00:36:13 +0000 (16:36 -0800)]
checkout, clone: die if tree cannot be parsed
When a tree oid is invalid, parse_tree_indirect() can return NULL. Check
for NULL instead of proceeding as though it were a valid pointer and
segfaulting.
Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
