Junio C Hamano [Mon, 16 Sep 2024 21:06:06 +0000 (14:06 -0700)]
Merge branch 'cp/unit-test-reftable-stack' into ps/reftable-alloc-failures
* cp/unit-test-reftable-stack:
t-reftable-stack: add test for stack iterators
t-reftable-stack: add test for non-default compaction factor
t-reftable-stack: use reftable_ref_record_equal() to compare ref records
t-reftable-stack: use Git's tempfile API instead of mkstemp()
t: harmonize t-reftable-stack.c with coding guidelines
t: move reftable/stack_test.c to the unit testing framework
refs/reftable: wire up support for exclude patterns
Exclude patterns can be used by reference backends to skip over blocks
of references that are uninteresting to the caller. Reference backends
do not have to wire up support for them, and all callers are expected to
behave as if the backend didn't support them. In fact, the only backend
that supports exclude patterns right now is the "packed" backend.
Exclude patterns can be quite an important performance optimization in
repositories that have loads of references. The patterns are set up in
case "transfer.hideRefs" and friends are configured during a fetch, so
handling these patterns becomes important once there are lots of hidden
refs in a served repository.
Now that we have properly re-seekable reftable iterators we can also
wire up support for these patterns in the "reftable" backend. Doing so
is conceptually simple: once we hit a reference whose prefix matches the
current exclude pattern we re-seek the iterator to the first reference
that doesn't match the pattern anymore. This schema only works for
trivial patterns that do not have any globbing characters in them, but
this restriction also applies do the "packed" backend.
This makes t1419 work with the "reftable" backend with some slight
modifications. Of course it also speeds up listing of references with
hidden refs. The following benchmark prints one reference with 1 million
hidden references:
Benchmark 1: HEAD~
Time (mean ± σ): 93.3 ms ± 2.1 ms [User: 90.3 ms, System: 2.5 ms]
Range (min … max): 89.8 ms … 97.2 ms 33 runs
Benchmark 2: HEAD
Time (mean ± σ): 4.2 ms ± 0.6 ms [User: 2.2 ms, System: 1.8 ms]
Range (min … max): 3.1 ms … 8.1 ms 765 runs
Summary
HEAD ran
22.15 ± 3.19 times faster than HEAD~
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 67ce50ba26 (Merge branch 'ps/reftable-reusable-iterator', 2024-05-30)
we have refactored the interface of reftable iterators such that they
can be reused in theory. This patch series only landed the required
changes on the interface level, but didn't yet implement the actual
logic to make iterators reusable.
As it turns out almost all of the infrastructure already does support
re-seeking. The only exception is the table iterator, which does not
reset its `is_finished` bit. Do so and add a couple of tests that verify
that we can re-seek iterators.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have recently migrated all of the reftable unit tests that were part
of the reftable library into our own unit testing framework. As part of
that migration we have duplicated some of the functionality that was
part of the reftable test framework into each of the migrated test
suites. This was a sensible decision to not have all of the migrations
dependent on each other, but now that the migration is done it makes
sense to deduplicate the functionality again.
Introduce a new reftable test library that hosts some shared code and
adapt tests to use it.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Whenever one adds another test library compilation unit one has to wire
it up twice in the Makefile: once to append it to `UNIT_TEST_OBJS`, and
once to append it to the `UNIT_TEST_PROGS` target. Ideally, we'd just
reuse the `UNIT_TEST_OBJS` variable in the target so that we can avoid
the duplication. But it also contains all the objects for our test
programs, each of which contains a `cmd_main()`, and thus we cannot link
them all into the target executable.
Refactor the code such that `UNIT_TEST_OBJS` does not contain the unit
test program objects anymore, which we can instead manually append to
the `OBJECTS` variable. Like this, the former variable now only contains
objects for test libraries and can thus be reused.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/receive-pack: fix exclude patterns when announcing refs
In `write_head_info()` we announce references to the remote client. We
need to honor "transfer.hideRefs" here so that we do not announce any
references that the client shouldn't be able to learn about. This is
done via two separate mechanisms:
- We hand over exclude patterns to the reference backend. We can only
honor "plain" exclude patterns here that do not have prefixes with
special meaning such as "^" or "!". Filtering down the references is
handled by `hidden_refs_to_excludes()`.
- In `show_ref_cb()` we perform a second check against hidden refs.
For one this is done such that we can handle those special prefixes.
And second, handling exclude patterns in ref backends is optional,
so we also have to handle "normal" patterns.
The special-meaning "^" prefix alters whether a hidden ref applies to
the namespace-stripped reference name or the full name. So while we
would usually call `refs_for_each_namespaced_ref()` to only get those
references in the current namespace, we can't because we'd get the
already-rewritten reference names. Instead, we are forced to use
`refs_for_each_fullref_in()` and then manually strip away the namespace
prefix such that we have access to both names.
But this also means that we do not get namespace handling for exclude
patterns, which `refs_for_each_namespaced_ref()` brings for free. This
results in a bug because we potentially end up hiding away references
based on their namespaced name and not on the stripped name as we really
should be doing.
Fix this by manually rewriting the exclude patterns to their namespaced
variants.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: properly apply exclude patterns to namespaced refs
Reference namespaces allow commands like git-upload-pack(1) to serve
different sets of references to the client depending on which namespace
is enabled, which is for example useful in fork networks. Namespaced
refs are stored with a `refs/namespaces/$namespace` prefix, but all the
user will ultimately see is a stripped version where that prefix is
removed.
The way that this interacts with "transfer.hideRefs" is not immediately
obvious: the hidden refs can either apply to the stripped references, or
to the non-stripped ones that still have the namespace prefix. In fact,
the "transfer.hideRefs" machinery does the former and applies to the
stripped reference by default, but rules can have "^" prefixed to switch
this behaviour to instead match against the full reference name.
Namespaces are exclusively handled at the generic "refs" layer, the
respective backends have no clue that such a thing even exists. This
also has the consequence that they cannot handle hiding references as
soon as reference namespaces come into play because they neither know
whether a namespace is active, nor do they know how to strip references
if they are active.
Handling such exclude patterns in `refs_for_each_namespaced_ref()` and
`refs_for_each_fullref_in_prefixes()` is broken though, as both support
that the user passes both namespaces and exclude patterns. In the case
where both are set we will exclude references with unstripped names,
even though we really wanted to exclude references based on their
stripped names.
This only surfaces when:
- A repository uses reference namespaces.
- "transfer.hideRefs" is active.
- The namespaced references are packed into the "packed-refs" file.
None of our tests exercise this scenario, and thus we haven't ever hit
it. While t5509 exercises both (1) and (2), it does not happen to hit
(3). It is trivial to demonstrate the bug though by explicitly packing
refs in the tests, and then we indeed surface the breakage.
Fix this bug by prefixing exclude patterns with the namespace in the
generic layer. The newly introduced function will be used outside of
"refs.c" in the next patch, so we add a declaration to "refs.h".
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sun, 15 Sep 2024 11:31:15 +0000 (07:31 -0400)]
t9001: use a more distinct fake BugID
In the test "cc list is sanitized", we feed a commit with a variety of
trailers to send-email, and then check its output to see how it handled
them. For most of them, we are grepping for a specific mention of the
header, but there's a "BugID" header which we expect to be ignored. We
confirm this by grepping for "12345", the fake BugID, and making sure it
is not present.
But we can be fooled by false positives! I just tracked down a flaky
test failure here that was caused by matching this unrelated line in the
output:
which will change from run to run based on the time, pid, etc.
Ideally we'd tighten the regex to make this more specifically, but since
the point is that it _shouldn't_ be mentioned, it's hard to say what the
right match would be (e.g., would there be a leading space?).
Instead, let's just choose a match that is much less likely to appear.
The actual content of the header isn't important, since it's supposed to
be ignored.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sun, 15 Sep 2024 11:20:24 +0000 (07:20 -0400)]
git-jump: ignore deleted files in diff mode
If you do something like this:
rm file_a
echo change >file_b
git jump diff
then we'll generate two quickfix entries for the diff, one for each
file. But the one for the deleted file is rather pointless. There's no
content to show since the file is gone, and in fact we open the editor
with the path /dev/null!
In vim, at least, the result is a confusing annoyance: the editor opens
with an empty buffer, and you have to skip past it to the useful
quickfix entry (after scratching your head and figuring out that no,
nothing is broken).
Let's skip such entries entirely. There's nothing useful to show, since
the point is that the file has been deleted.
It is possible that you could be doing a diff whose post-image is not
the working tree, and then you'd perhaps be jumping to the deleted
content (or at least something that was in the same spot). But I don't
think it's worth worrying about that case. For one thing, using git-jump
for such diffs is a bad idea in general, as it's going to sometimes move
you to the wrong spot. And two, a deletion is always going to have one
hunk starting at line 1, which is not that interesting to jump to in the
first place.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
which is ambiguous. It could be line 1 of "file", or line 11 of the file
"file:1: static_lease 10", and so on. In the case of vim's default
config, it seems to prefer the latter (you can configure "errorformat"
with a variety of patterns, but out of the box it matches some common
ones).
One easy way to fix this is to provide a column number, like:
also works. That perhaps would the right thing even if you had the silly
file name "file:1:1: foo 10". But it's not clear what would happen if
you had a filename with quotes in it.
This feature is inherently scraping text, and there's bound to be some
ambiguities. I don't think it's worth worrying too much about unlikely
filenames, as its the file content that is more likely to introduce
unexpected characters.
So let's just go with the extra ":1" column specifier. We know this is
supported everywhere, as git-jump's "grep" mode already uses it (and
thus doesn't exhibit the same problem).
The "merge" mode is mostly immune to this, as it only matches "<<<<<<<"
conflict marker lines. It's possible of course to have a marker that
says "foo 10:11" later in the line, but in practice these will only have
branches and perhaps file names, so it's probably not worth worrying
about (and fixing it would involve passing --column to the system grep,
which may not be portable).
I also gave some thought as to whether we could put something more
useful than "1" in the column field for diffs. In theory we could find
the first changed character of the line, but this is tricky in practice.
You'd have to correlate before/after lines of the hunk to decide what
changed. So:
-this is a foo line
+this is a bar line
is easy (column 11). But:
-this is a foo line
+another line
+this is a bar line
is harder.
This commit certainly doesn't preclude trying to do something more
clever later, but it's a much deeper rabbit hole than just fixing the
syntactic ambiguity.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Fri, 17 Jan 2014 20:36:21 +0000 (15:36 -0500)]
print an error when remote helpers die during capabilities
The transport-helper code generally relies on the
remote-helper to provide an informative message to the user
when it encounters an error. In the rare cases where the
helper does not do so, the output can be quite confusing.
E.g.:
$ git clone https://example.com/foo.git
Cloning into 'foo'...
$ echo $?
128
$ ls foo
/bin/ls: cannot access foo: No such file or directory
We tried to address this with 81d340d (transport-helper:
report errors properly, 2013-04-10).
But that makes the common case much more confusing. The
remote helper protocol's method for signaling normal errors
is to simply hang up. So when the helper does encounter a
routine error and prints something to stderr, the extra
error message is redundant and misleading. So we dropped it
again in 266f1fd (transport-helper: be quiet on read errors
from helpers, 2013-06-21).
This puts the uncommon case right back where it started. We
may be able to do a little better, though. It is common for
the helper to die during a "real" command, like fetching the
list of remote refs. It is not common for it to die during
the initial "capabilities" negotiation, right after we
start. Reporting failure here is likely to catch fundamental
problems that prevent the helper from running (and reporting
errors) at all. Anything after that is the responsibility of
the helper itself to report.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git gui: add directly calling merge tool from configuration
git gui can open a merge tool when conflicts are detected (Right click
in the diff of the file with conflicts).
The merge tools that are allowed to use are hard coded into git gui.
If one wants to add a new merge tool it has to be added to git gui
through a source code change.
This is not convenient in comparison to how it works in git (without gui).
git itself has configuration options for a merge tools path and command
in the git configuration.
New merge tools can be set up there without a source code change.
Those options are used only by pure git in contrast to git gui. git calls
the configured merge tools directly from the configuration while git Gui
doesn't.
With this change git gui can call merge tools configured in the
configuration directly without a change in git gui source code.
It needs a configured "merge.tool" and a configured
"mergetool.<mergetool name>.cmd" configuration entry as shown in the
git-config manual page.
Without the "mergetool.<mergetool name>.cmd" entry and an unsupported
"merge.tool" entry, git gui behaves mainly as before this change and
informs the user about an unsupported merge tool. In addtition, it also
shows a hint to add a configuration entry to use the tool as an
unsupported tool with degraded support.
If a wrong "mergetool.<mergetool name>.cmd" is configured by accident,
it gets handled by git gui already. In this case git gui informs the
user that the merge tool couldn't be opened. This behavior is preserved
by this change and should not change.
"Beyond Compare 3" and "Visual Studio Code" were tested as manually
configured merge tools.
Signed-off-by: Tobias Boesch <tobias.boesch@miele.com> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
The environment GIT_ADVICE has been intentionally kept undocumented
to discourage its use by interactive users. Add documentation to
help tool writers.
* ds/doc-wholesale-disabling-advice-messages:
advice: recommend GIT_ADVICE=0 for tools
Junio C Hamano [Fri, 13 Sep 2024 22:27:42 +0000 (15:27 -0700)]
Merge branch 'jk/sparse-fdleak-fix'
A file descriptor left open is now properly closed when "git
sparse-checkout" updates the sparse patterns.
* jk/sparse-fdleak-fix:
sparse-checkout: use fdopen_lock_file() instead of xfdopen()
sparse-checkout: check commit_lock_file when writing patterns
sparse-checkout: consolidate cleanup when writing patterns
Junio C Hamano [Fri, 13 Sep 2024 22:26:49 +0000 (15:26 -0700)]
Merge branch 'jk/free-commit-buffer-of-skipped-commits' into maint-2.46
The code forgot to discard unnecessary in-core commit buffer data
for commits that "git log --skip=<number>" traversed but omitted
from the output, which has been corrected.
* jk/free-commit-buffer-of-skipped-commits:
revision: free commit buffers for skipped commits
John Cai [Fri, 13 Sep 2024 21:16:17 +0000 (21:16 +0000)]
add: pass in repo variable instead of global the_repository
With the repository variable available in the builtin function as an
argument, pass this down into helper functions instead of using the
global the_repository.
Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
John Cai [Fri, 13 Sep 2024 21:16:15 +0000 (21:16 +0000)]
builtin: remove USE_THE_REPOSITORY_VARIABLE from builtin.h
Instead of including USE_THE_REPOSITORY_VARIABLE by default on every
builtin, remove it from builtin.h and add it to all the builtins that
include builtin.h (by definition, that means all builtins/*.c).
Also, remove the include statement for repository.h since it gets
brought in through builtin.h.
The next step will be to migrate each builtin
from having to use the_repository.
Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Fri, 13 Sep 2024 19:26:41 +0000 (12:26 -0700)]
t5512.40 sometimes dies by SIGPIPE
The last test in t5512 we recently added seems to be flaky.
Running
$ make && cd t && sh ./t5512-ls-remote.sh --stress
shows that "git ls-remote foo::bar" exited with status 141, which
means we got a SIGPIPE. This test piece was introduced by 9e89dcb6
(builtin/ls-remote: fall back to SHA1 outside of a repo, 2024-08-02)
and is pretty much independent from all other tests in the script
(it can even run standalone with everything before it removed).
The transport-helper.c:get_helper() function tries to write to the
helper. As we can see the helper script is very short and can exit
even before it reads anything, when get_helper() tries to give the
first command, "capabilities", the helper may already be gone.
A trivial fix, presented here, is to make sure that the helper reads
the first command it is given, as what it writes later is a response
to that command.
I however would wonder if the interactions with the helper initiated
by get_helper() should be done on a non-blocking I/O (we do check
the return value from our write(2) system calls, do we?).
Jeff King [Thu, 12 Sep 2024 22:37:25 +0000 (18:37 -0400)]
Git.pm: use "rev-parse --absolute-git-dir" rather than perl code
When we open a repository with the "Directory" option, we use "rev-parse
--git-dir" to get the path relative to that directory, and then use
Cwd::abs_path() to make it absolute (since our process working directory
may not be the same).
These days we can just ask for "--absolute-git-dir" instead, which saves
us a little code. That option was added in Git v2.13.0 via a2f5a87626
(rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think
we make any promises about running mismatched versions of git and
Git.pm, but even if somebody tries it, that's sufficiently old that it
should be OK.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
we will incorrectly point the repository object at the _current_
directory, not the one specified by the option.
The bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find
bare repositories, 2022-10-22). Before then, we'd ask "rev-parse
--git-dir" if it was a Git repo, and if it returned anything, we'd
correctly convert that result to an absolute path using File::Spec and
Cwd::abs_path(). If it didn't, we'd guess it might be a bare repository
and find it ourselves, which was wrong (rev-parse should find even a
bare repo, and our search circumvented some of its rules).
That commit dropped most of the custom bare-repo search code in favor of
using "rev-parse --is-bare-repository" and trusting the "--git-dir" it
returned. But it mistakenly left some of the bare-repo code path in
place, which was now broken. That code calls Cwd::abs_path($dir); prior
to 20da61f25f $dir contained the "Directory" option the user passed in.
But afterwards, it contains the output of "rev-parse --git-dir". And
since our tentative rev-parse command is invoked after changing
directory, it will always be the relative path "."! So we'll end up with
the absolute path of the process's current directory, not the Directory
option the caller asked for.
So the non-bare case is correct, but the bare one is broken. Our tests
only check the non-bare one, so we didn't notice. We can fix this by
running the same absolute-path fixup code for both sides.
Helped-by: Rodrigo <rodrigolive@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commits we had to convert the linux32 job to be based
on Ubuntu 20.04 instead of Ubuntu 16.04 due to a limitation in GitHub
Workflows. This was the only job left that still tested against this old
but supported Ubuntu version, and we have no other jobs that test with a
comparatively old Linux distribution.
Add a new job to GitLab CI that tests with Ubuntu 16.04 to cover the
resulting test gap. GitLab doesn't modify Docker images in the same way
GitHub does and thus doesn't fall prey to the same issue. There are two
compatibility issues uncovered by this:
- Ubuntu 16.04 does not support HTTP/2 in Apache. We thus cannot set
`GIT_TEST_HTTPD=true`, which would otherwise cause us to fail when
Apache fails to start.
- Ubuntu 16.04 cannot use recent JGit versions as they depend on a
more recent Java runtime than we have available. We thus disable
installing any kind of optional dependencies that do not come from
the package manager.
These two restrictions are fine though, as we only really care about
whether Git compiles and runs on such old distributions in the first
place.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Wed, 11 Sep 2024 06:10:09 +0000 (02:10 -0400)]
t/interop: allow per-version make options
Building older versions of Git may require tweaking some build knobs. In
particular, very old versions of Git will fail to build with recent
OpenSSL, because the bignum type switched from a struct to a pointer.
The i5500 interop test uses Git v1.0.0 by default, which triggers this
problem. You can work around it by setting NO_OPENSSL in your
GIT_TEST_MAKE_OPTS variable. But there are two downsides:
1. You have to know to do this, and it's not at all obvious.
2. That sets the options for _all_ versions of Git that we build. And
it's possible for two versions to require conflicting knobs. E.g.,
building with "make NO_OPENSSL=Nope OPENSSL_SHA1=Yes" causes
imap-send.c to barf, because it declares a fallback typedef for SSL.
This is something we may want to fix, but of course many historical
versions are affected, and the interop scripts should be flexible
enough to build everything.
So let's introduce per-version make options, along with the ability for
scripts to specify knobs that match their default versions. That should
make everything build out of the box, but also allow testers flexibility
if they are testing interoperability between non-default versions.
We'll set NO_OPENSSL by default for v1.0.0 in i5500. It doesn't have to
worry about the conflict with OPENSSL_SHA1 because imap-send did not
exist back then (but if it did, it could also just explicitly use a
different hash implementation).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 12 Sep 2024 18:47:23 +0000 (11:47 -0700)]
Merge branch 'jk/messages-with-excess-lf-fix'
One-line messages to "die" and other helper functions will get LF
added by these helper functions, but many existing messages had an
unnecessary LF at the end, which have been corrected.
* jk/messages-with-excess-lf-fix:
drop trailing newline from warning/error/die messages
Junio C Hamano [Thu, 12 Sep 2024 18:47:23 +0000 (11:47 -0700)]
Merge branch 'ps/pack-refs-auto-heuristics'
"git pack-refs --auto" for the files backend was too aggressive,
which has been a bit tamed.
* ps/pack-refs-auto-heuristics:
refs/files: use heuristic to decide whether to repack with `--auto`
t0601: merge tests for auto-packing of refs
wrapper: introduce `log2u()`
Junio C Hamano [Thu, 12 Sep 2024 18:47:23 +0000 (11:47 -0700)]
Merge branch 'tb/multi-pack-reuse-fix'
A data corruption bug when multi-pack-index is used and the same
objects are stored in multiple packfiles has been corrected.
* tb/multi-pack-reuse-fix:
builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`
pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse
builtin/pack-objects.c: translate bit positions during pack-reuse
pack-bitmap: tag bitmapped packs with their corresponding MIDX
t/t5332-multi-pack-reuse.sh: verify pack generation with --strict
Junio C Hamano [Thu, 12 Sep 2024 18:02:16 +0000 (11:02 -0700)]
Merge branch 'ps/bundle-outside-repo-fix' into maint-2.46
"git bundle unbundle" outside a repository triggered a BUG()
unnecessarily, which has been corrected.
* ps/bundle-outside-repo-fix:
bundle: default to SHA1 when reading bundle headers
builtin/bundle: have unbundle check for repo before opening its bundle
Junio C Hamano [Thu, 12 Sep 2024 18:02:16 +0000 (11:02 -0700)]
Merge branch 'jc/patch-id' into maint-2.46
The patch parser in "git patch-id" has been tightened to avoid
getting confused by lines that look like a patch header in the log
message.
cf. <Zqh2T_2RLt0SeKF7@tanuki>
* jc/patch-id:
patch-id: tighten code to detect the patch header
patch-id: rewrite code that detects the beginning of a patch
patch-id: make get_one_patchid() more extensible
patch-id: call flush_current_id() only when needed
t4204: patch-id supports various input format
Jeff King [Wed, 11 Sep 2024 06:12:57 +0000 (02:12 -0400)]
imap-send: handle NO_OPENSSL even when openssl exists
If NO_OPENSSL is defined, then imap-send.c defines a fallback "SSL"
type, which is just a void pointer that remains NULL. This works, but it
has one problem: it is using the type name "SSL", which conflicts with
the upstream name, if some other part of the system happens to include
openssl. For example:
$ make NO_OPENSSL=Nope OPENSSL_SHA1=Yes imap-send.o
CC imap-send.o
imap-send.c:35:15: error: conflicting types for ‘SSL’; have ‘void *’
35 | typedef void *SSL;
| ^~~
In file included from /usr/include/openssl/evp.h:26,
from sha1/openssl.h:4,
from hash.h:10,
from object.h:4,
from commit.h:4,
from refs.h:4,
from setup.h:4,
from imap-send.c:32:
/usr/include/openssl/types.h:187:23: note: previous declaration of ‘SSL’ with type ‘SSL’ {aka ‘struct ssl_st’}
187 | typedef struct ssl_st SSL;
| ^~~
make: *** [Makefile:2761: imap-send.o] Error 1
This is not a terribly common combination in practice:
1. Why are we disabling openssl support but still using its sha1? The
answer is that you may use the same build options across many
versions, and some older versions of Git no longer build with
modern versions of openssl.
2. Why are we using a totally unsafe sha1 that does not detect
collisions? You're right, we shouldn't. But in preparation for
using unsafe sha1 for non-cryptographic checksums, it would be nice
to be able to turn it on without hassle.
We can make this work by adjusting the way imap-send handles its
fallback. One solution is something like this:
But we can observe that we only need this definition in one spot: the
struct which holds the variable. So rather than play around with macros
that may cause unexpected effects, we can just directly use the correct
type in that struct.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Thu, 12 Sep 2024 09:48:41 +0000 (05:48 -0400)]
ci: use regular action versions for linux32 job
The linux32 job runs inside a docker container with a 32-bit libc, etc.
This breaks any GitHub Actions scripts that are implemented in
javascript, because they ship with their own 64-bit version of Node.js
that's dynamically linked. They'll fail with a message like:
exec /__e/node20/bin/node: no such file or directory
because they can't find the runtime linker.
This hasn't been a problem until recently because we special-case older,
non-javascript versions of these actions for the linux32 job. But it
recently became an issue when our old version of actions/upload-artifact
was deprecated, causing the job to fail. We worked around that in 90f2c7240c (ci: remove 'Upload failed tests' directories' step from
linux32 jobs, 2024-09-09), but it meant a loss of functionality for that
job. And we may eventually run into the same deprecation problem with
actions/checkout, which can't just be removed.
We can solve the linking issue by installing the 64-bit libc and stdc++
packages before doing anything else. Coupled with the switch to a more
recent image in the previous patch, that lets us remove the
special-casing of the action scripts entirely.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Thu, 12 Sep 2024 09:47:30 +0000 (05:47 -0400)]
ci: use more recent linux32 image
The Xenial image we're using was released more than 8 years ago. This is
a problem for using some recent GitHub Actions scripts, as they require
Node.js 20, and all of the binaries they ship need glibc 2.28 or later.
We're not using them yet, but moving forward prepares us for a future
patch which will.
Xenial was actually the last official 32-bit Ubuntu release, but you can
still find i386 images for more recent releases. This patch uses Focal,
which was released in 2020 (and is the oldest one with glibc 2.28).
There are two small downsides here:
- while Xenial is pretty old, it is still in LTS support until April
2026. So there's probably some value in testing with such an old
system, and we're losing that.
- there are no i386 subversion packages in the Focal repository. So we
won't be able to test that (OTOH, we had never tested it until the
previous patch which unified the 32/64-bit dependency code).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Thu, 12 Sep 2024 09:45:37 +0000 (05:45 -0400)]
ci: unify ubuntu and ubuntu32 dependencies
The script to install dependencies has two separate entries for 32-bit
and 64-bit Ubuntu systems. This increases the maintenance burden since
both should need roughly the same packages.
That hasn't been too bad so far because we've stayed on the same 32-bit
image since 2017. Trying to move to a newer image revealed several
problems with the linux32 job:
- newer images complain about using "linux32 --32bit i386", due to
seccomp restrictions. We can loosen these with a docker option, but
I don't think running it is even doing anything. We use it only for
pretending to "apt" that we're on a 32-bit machine, but inside the
container image apt is already configured as a 32-bit system (even
though the kernel outside the container is obviously 64-bit). Using
the same apt invocation for both architectures just gets rid of this
call entirely.
- we set DEBIAN_FRONTEND to avoid hanging on packages that ask the
user questions. This wasn't a problem on the old image, but it is on
newer ones. The 64-bit stanza handles this already.
As a bonus, the 64-bit stanza uses "apt -q" instead of redirecting
output to /dev/null. This would have saved me a lot of debugging
time trying to figure out why it was hanging. :)
- the old image seems to have zlib-dev installed by default, but newer
ones do not.
In addition, there were probably many tests being skipped on the 32-bit
build because we didn't have support packages installed (e.g., gpg). Now
we'll run them.
We do need to keep some parts split off just for 64-bit systems: our p4
and lfs installs reference x86_64/amd64 binaries. The downloaded jgit
should work in theory, since it's just a jar file embedded in a shell
script that relies on the system java. But the system java in our image
is too old, so I've left it as 64-bit only for now.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Thu, 12 Sep 2024 09:43:36 +0000 (05:43 -0400)]
ci: drop run-docker scripts
We haven't used these scripts since 4a6e4b9602 (CI: remove Travis CI
support, 2021-11-23), as the GitHub Actions config has support for
directly running jobs within docker containers.
It's possible we might want to resurrect something like this in order to
be more agnostic to the CI platform. But it's not clear exactly what it
would look like. And in the meantime, it's just a maintenance burden as
we make changes to CI config, and is subject to bitrot. In fact it's
already broken; it references ci/install-docker-dependencies.sh, which
went away in 9cdeb34b96 (ci: merge scripts which install dependencies,
2024-04-12).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Stop storing the "core.notesRef" config value globally. Instead,
retrieve the value in `default_notes_ref()`. The code is never called in
a hot loop anyway, so doing this on every invocation should be perfectly
fine.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Same as the preceding commits, storing the "core.warnAmbiguousRefs"
value globally is misdesigned as this setting may be set per repository.
Move the logic into the repo-settings subsystem. The usual pattern here
is that users are expected to call `prepare_repo_settings()` before they
access the settings themselves. This seems somewhat fragile though, as
it is easy to miss and leads to somewhat ugly code patterns at the call
sites.
Instead, introduce a new function that encapsulates this logic for us.
This also allows us to change how exactly the lazy initialization works
in the future, e.g. by only partially initializing values as requested
by the caller.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Same as the preceding commit, storing the "core.preferSymlinkRefs" value
globally is misdesigned as this setting may be set per repository.
There is only a single user of this value anyway, namely the "files"
backend. So let's just remove the global variable and read the value of
this setting when initializing the backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The value of "core.logAllRefUpdates" is being stored in the global
variable `log_all_ref_updates`. This design is somewhat aged nowadays,
where it is entirely possible to access multiple repositories in the
same process which all have different values for this setting. So using
a single global variable to track it is plain wrong.
Remove the global variable. Instead, we now provide a new function part
of the repo-settings subsystem that parses the value for a specific
repository. While that may require us to read the value multiple times,
we work around this by reading it once when the ref backends are set up
and caching the value there.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: stop modifying global `log_all_ref_updates` variable
In refs-related code we modify the global `log_all_ref_updates`
variable, which is done because `should_autocreate_reflog()` does not
accept passing an `enum log_refs_config` but instead accesses the global
variable. Adapt its interface such that the value is provided by the
caller, which allows us to compute the proper value locally without
having to modify global state.
This change requires us to move the enum to "repo-settings.h", or
otherwise we get compilation errors due to include cycles. We're about
to fully move this setting into the repo-settings subsystem anyway, so
this is fine.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "branch.c" we modify the global `log_all_ref_updates` variable to
force creation of a reflog entry. Modifying global state like this is
discouraged, as it may have all kinds of consequences in other places of
our codebase.
Stop modifying the variable and pass the `REF_FORCE_CREATE_REFLOG` flag
instead. Setting this flag has a stronger meaning than setting the
config to `LOG_REFS_NORMAL`:
- `LOG_REFS_NORMAL` will ask us to only create reflog entries for
preexisting reflogs or branches, remote refs, note refs and HEAD.
- `REF_FORCE_CREATE_REFLOG` will unconditionally create a reflog and
is thus equivalent to `LOG_REFS_ALWAYS`.
But as we are in `create_branch()` and thus do not have to worry about
arbitrary references, but only about branches, `LOG_REFS_NORMAL` and
`LOG_REFS_ALWAYS` are indeed equivalent.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
repo-settings: track defaults close to `struct repo_settings`
The default values for `struct repo_settings` are set up in
`prepare_repo_settings()`. This is somewhat different from how we
typically do this, namely by providing an `INIT` macro that sets up the
default values for us.
Refactor the code to do the same.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
repo-settings: split out declarations into a standalone header
While we have "repo-settings.c", we do not have a corresponding
"repo-settings.h" file. Instead, this functionality is part of the
"repository.h" header, making it hard to discover.
Split the declarations out of "repository.h" and create a standalone
header file with them.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: guard state depending on a repository
In "environment.h" we have quite a lot of functions and variables that
either explicitly or implicitly depend on `the_repository`.
The implicit set of stateful declarations includes for example variables
which get populated when parsing a repository's Git configuration. This
set of variables is broken by design, as their state often depends on
the last repository config that has been parsed. So they may or may not
represent the state of `the_repository`.
Fixing that is quite a big undertaking, and later patches in this series
will demonstrate a solution for a first small set of those variables. So
for now, let's guard these with `USE_THE_REPOSITORY_VARIABLE` so that
callers are aware of the implicit dependency.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: reorder header to split out `the_repository`-free section
Reorder the "environment.h" header such that declarations which are free
from `the_repository` come before those which aren't. The new structure
is now:
- Defines for environment variable names.
- Things which do not rely on a repository.
- Things which do, including those that implicitly rely on a parsed
repository. This includes for example variables which get
populated when reading repository config.
This will allow us to guard the last category of declarations with
`USE_THE_REPOSITORY_VARIABLE`.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: move `set_git_dir()` and related into setup layer
The functions `set_git_dir()` and friends are used to set up
repositories. As such, they are quite clearly part of the setup
subsystem, but still live in "environment.c". Move them over, which also
helps to get rid of dependencies on `the_repository` in the environment
subsystem.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_git_namespace()` self-contained
The logic to set up and retrieve `git_namespace` is distributed across
different functions which communicate with each other via a global
environment variable. This is rather pointless though, as the value is
always derived from an environment variable, and this environment
variable does not change after we have parsed global options.
Convert the function to be fully self-contained such that it lazily
populates once called.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: move object database functions into object layer
The `odb_mkstemp()` and `odb_pack_keep()` functions are quite clearly
tied to the object store, but regardless of that they are located in
"environment.c". Move them over, which also helps to get rid of
dependencies on `the_repository` in the environment subsystem.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
config: make dependency on repo in `read_early_config()` explicit
The `read_early_config()` function can be used to read configuration
where a repository has not yet been set up. As such, it is optional
whether or not `the_repository` has already been initialized. If it was
initialized we use its commondir and gitdir. If not, the function will
try to detect the Git directories by itself and, if found, also parse
their config files.
This means that we implicitly rely on `the_repository`. Make this
dependency explicit by passing a `struct repository`. This allows us to
again drop the `USE_THE_REPOSITORY_VARIABLE` define in "config.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
config: document `read_early_config()` and `read_very_early_config()`
It's not clear what `read_early_config()` and `read_very_early_config()`
do differently compared to `repo_read_config()` from just looking at
their names. Document both of these in the header file to clarify their
intent.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_git_work_tree()` accept a repository
The `get_git_work_tree()` function retrieves the path of the work tree
of `the_repository`. Make it accept a `struct repository` such that it
can work on arbitrary repositories and make it part of the repository
subsystem. This reduces our reliance on `the_repository` and clarifies
scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_graft_file()` accept a repository
The `get_graft_file()` function retrieves the path to the graft file of
`the_repository`. Make it accept a `struct repository` such that it can
work on arbitrary repositories and make it part of the repository
subsystem. This reduces our reliance on `the_repository` and clarifies
scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_index_file()` accept a repository
The `get_index_file()` function retrieves the path to the index file
of `the_repository`. Make it accept a `struct repository` such that it
can work on arbitrary repositories and make it part of the repository
subsystem. This reduces our reliance on `the_repository` and clarifies
scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_object_directory()` accept a repository
The `get_object_directory()` function retrieves the path to the object
directory for `the_repository`. Make it accept a `struct repository`
such that it can work on arbitrary repositories and make it part of the
repository subsystem. This reduces our reliance on `the_repository` and
clarifies scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_git_common_dir()` accept a repository
The `get_git_common_dir()` function retrieves the path to the common
directory for `the_repository`. Make it accept a `struct repository`
such that it can work on arbitrary repositories and make it part of the
repository subsystem. This reduces our reliance on `the_repository` and
clarifies scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
environment: make `get_git_dir()` accept a repository
The `get_git_dir()` function retrieves the path to the Git directory for
`the_repository`. Make it accept a `struct repository` such that it can
work on arbitrary repositories and make it part of the repository
subsystem. This reduces our reliance on `the_repository` and clarifies
scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Martin Ågren [Wed, 11 Sep 2024 10:31:00 +0000 (12:31 +0200)]
t0211: add missing LIBCURL prereq
After building Git with NO_LIBCURL, we're lacking `git remote-http` and
`git http-fetch`, so when we test that they trace as they should, we're
bound to fail. Add the LIBCURL prereq to those tests.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Martin Ågren [Wed, 11 Sep 2024 10:30:59 +0000 (12:30 +0200)]
t1517: add missing LIBCURL prereq
After building Git with NO_LIBCURL, there is no `git remote-http`, so
it's not meaningful to test that it can run outside of a repository.
Indeed, that test will fail. Add the LIBCURL prereq to it.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The code forgot to discard unnecessary in-core commit buffer data
for commits that "git log --skip=<number>" traversed but omitted
from the output, which has been corrected.
* jk/free-commit-buffer-of-skipped-commits:
revision: free commit buffers for skipped commits
Makefile: rename clar-related variables to avoid confusion
The Makefile variables related to the recently-introduced clar testing
framework have a `UNIT_TESTS_` prefix. This prefix is extremely similar
to the prefix used by our other unit tests that use our homegrown unit
testing framework, which is `UNIT_TEST_`. The consequence is that it is
easy to misread the names and confuse them with each other.
Rename the clar-related variables to instead have a `CLAR_TEST_` prefix
to address this.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine [Tue, 10 Sep 2024 04:10:13 +0000 (00:10 -0400)]
chainlint: reduce annotation noise-factor
When chainlint detects a problem in a test definition, it highlights the
offending code with a "?!...?!" annotation. The rather curious "?!"
decoration was chosen to draw the reader's attention to the problem area
and to act as a good "needle" when using the terminal's search feature
to "jump" to the next problem.
Later, chainlint learned to color its output when sent to a terminal.
Problem annotations are colored with a red background which stands out
well from surrounding text, thus easily draws the reader's attention.
Together with the preceding change which gave all problem annotations a
uniform "LINT:" prefix, the noisy "?!" decoration has become superfluous
as a search "needle" so omit it when output is colored.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine [Tue, 10 Sep 2024 04:10:12 +0000 (00:10 -0400)]
chainlint: make error messages self-explanatory
The annotations emitted by chainlint to indicate detected problems are
overly terse, so much so that developers new to the project -- those who
should most benefit from the linting -- may find them baffling. For
instance, although the author of chainlint and seasoned Git developers
may understand that "?!AMP?!" is an abbreviation of "ampersand" and
indicates a break in the &&-chain, this may not be obvious to newcomers.
The "?!LOOP?!" case is particularly serious because that terse single
word does nothing to convey that the loop body should end with
"|| return 1" (or "|| exit 1" in a subshell) to ensure that a failing
command in the body aborts the loop immediately. Moreover, unlike
&&-chaining which is ubiquitous in Git tests, the "|| return 1" idiom is
relatively infrequent, thus may be harder for a newcomer to discover by
consulting nearby code.
Address these shortcomings by emitting human-readable messages which
both explain the problem and give a strong hint about how to correct it.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine [Tue, 10 Sep 2024 04:10:11 +0000 (00:10 -0400)]
chainlint: don't be fooled by "?!...?!" in test body
As originally implemented, chainlint did not collect structured
information about detected problems. Instead, it merely emitted raw
parse tokens (not the original test text), along with a "?!...?!"
annotation directly into the output stream each time a problem was
discovered. In order to report statistics (in --stats mode) and to
adjust its exit code to indicate success or failure, it merely counts
the number of times "?!...?!" appears in the output stream. An obvious
shortcoming of this approach is that it can be fooled by a legitimate
"?!...?!" sequence in the body of a test (though, only if an actual
problem is detected in the test).
The situation did not improve when 7c04aa7390 (chainlint: colorize
problem annotations and test delimiters, 2022-09-13) colored the
annotations after-the-fact by searching for "?!...?!" in the output
stream and inserting color codes. As above, a shortcoming is that this
approach can incorrectly color a legitimate "?!...?!" sequence in a test
body as if it is an error.
However, when 73c768dae9 (chainlint: annotate original test definition
rather than token stream, 2022-11-08) taught chainlint to output the
original test text verbatim, it started collecting structured
information about detected problems.
Now that it is available, take advantage of the structured problem
information to deterministically count the number of problems detected
and to color the annotations directly, rather than scanning the output
stream for "?!...?!" and performing these operations after-the-fact.
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ref-filter: fix leak with unterminated %(if) atoms
When parsing `%(if)` atoms we expect a few other atoms to exist to
complete it, like `%(then)` and `%(end)`. Whether or not we have seen
these other atoms is tracked in an allocated `if_then_else` structure,
which gets free'd by the `if_then_else_handler()` once we have parsed
the complete conditional expression.
This results in a memory leak when the `%(if)` atom is not terminated
correctly and thus incomplete. We never end up executing its handler and
thus don't end up freeing the structure.
Plug this memory leak by introducing a new `at_end_data_free` callback
function. If set, we'll execute it in `pop_stack_element()` and pass it
the `at_end_data` variable with the intent to free its state. Wire it up
for the `%(if)` atom accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
